-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fullscreen Mode: Change W button to toggle wp-admin sidebar #22191
Conversation
Using this feels solid. I only have minor feedback: In Chromium (Chrome and Edge Mac), I noticed there is an outline style around the W when clicked. It does not show up in Firefox. It made me think I had to click within the blue border to close it. The other minor weirdness is around the speed of it closing. When I click in to the editor, it closes automatically, which is great. It does feel like it takes a touch too long. The timing might be good to explore in a future issue/PR (not this one). |
I couldn't see a linked issue, and #22178 was reported, so I've linked this PR as closing that issue. |
89f63a4
to
d124acc
Compare
Size Change: +11.9 kB (1%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
Seems like there's a coupe bugs here The safari issue is back (white space) I think this state was one of the reasons the revert was suggested, should we hide the "Collapse menu" button on Fullscreen? Also from that state, when you click the "w" again, you get this. I think one of the other reasons, the revert was suggested was whether the "W" should be centered or not when the panel is open and potentially add the site title there? cc @mtias |
Thanks @youknowriad — some of these I knew, some I didn't.
I don't have a strong preference on the centering, but I also haven't heard it brought up before. Adding a site title was explored, but I still think its best left for another PR so we can figure out a good way to displaying titles without aggressive truncation. There was mention of having to integrate the wp-admin color schemes to the W button, and the lack of a selected state for the button. I'm still working on both of those. |
Thanks for your endurance, Shaun. It feels very important to me that users can middle-click to open in a new tab on the logo. For people who do this, they expect to be able to, and when they can't, it can erode a bit of trust. In the before-times, we would keep the The focus style doesn't meet contrast ratio. You could either do the double-shot version we have in master, or change the focus style to be white there. I'm happy to see you were able to get the sidebar scrolling on short viewports, or admins with a lot of menu items: It's unfortunate that we still have to have the double-scrollbar on the right to make this happen; the fact that we got rid of that double scrollbar with fullscreen was my personal favorite feature of that. But at least now it's consistent with non-fullscreen. If your neurons can think up a way to make the left navigation rail scroll independantly, as a separate PR to this, I would buy you a milkshake. No, make that two milkshakes. |
I haven't forgotten about this; Next on my list here.
I added a second, white border similar to what
I pushed a (hopeful) fix for this; Basically, I forced -- I managed to update the colors to respect the wp-admin color scheme settings, which required adding two new values to the -- I'm also still considering wether the "folded" or "collapsed" mode of the sidebar makes sense in this context. |
I think we should try just the white, this feels a little heavy: There's also a I also wonder whether it's actually nice for the focus to encompass the entire button rather than try to frame just the logo. Like so: That should translate to this:
I noticed a weird focus trap of sorts, though: In short, the tabbing context changes depending on whether the menu is open or not. If the menu is closed, you tab the top menu. If the menu is open, you tab through the menu. Both these seem fine, the problem is I can't shift tab backwards from the menu into the logo anymore. Not sure what the best practice is here.
This seems to work as intended! Still not ideal, but I don't think it can ever become ideal until we are able to touch the left navigation side rail directly. And that, I think, actually the biggest challenge here — it's bridging old and new, and the edgecases that pop up, including what you mention about folded cases and such, are really really difficult to address in a good way. I'm impressed that you're tackling it head-on! |
990422a
to
5c749db
Compare
👋 Haiii!! I can help out with the JS/interaction side of things :) @jasmussen Feedback/questions for you!
Gotcha. So allow for the W logo to work like a link. For example, when I edit a post, the URL is:
We can accommodate this. Just a heads up, the solution will be hacky. It's already quite hacky, but it'll be hackier. There isn't a way around this as the (React) Thanks! |
I would use the same heuristic as we have today, it has felt natural for me for the duration of using fullscreen. I'm not 100% on what that heuristic is, I think it's posts when you edit posts, pages when you edit pages, and probably cpts work as well. So to an extent, I think of this more as "restoring the current behavior".
Let's be liberal with code comments, and let's also take notes for how we could do this better. I hope that we'll sooner rather than later be able to revisit the actual side navigation rail to improve things there, and if/when we do it would be good to know what to refactor to make things better. |
@@ -92,6 +97,9 @@ function Editor( { settings: _settings } ) { | |||
<> | |||
<EditorStyles styles={ settings.styles } /> | |||
<FullscreenMode isActive={ isFullscreenActive } /> | |||
<MainDashboardButton.Slot> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried testing this with instructions from #22323 to verify if extensibility of the close button, and it doesn't seem to work anymore. The default button is not replaced.
I'm not sure what's causing this, and whether it's somehow related to moving this outside of editor skeleton. FWIW I don't think that it should be moved out of this area. With this setup it seems that everyone who's aiming to provide a custom button, will also have to provide styles to position it properly on top of the header, and I think that will be prone to breakage over time.
@jasmussen + @shaunandrews Haiii! I just pushed some updates that handles the 2 enhanced interactions:
I've added a bunch of comments that describe how it works, and acknowledging that it's perhaps not the best 😅 Let me know how it feels! |
@ItsJonQ Love the updates; Thanks! My only bit of feedback is that the I tried this locally, but for some reason I'm getting |
@shaunandrews Thank you! I'm happy to change the URL of the Thank you! |
Sorry, the link highlights a deleted file. Its strange. I copy/pasted the old button code:
|
@shaunandrews Awesome! Thank you! I'll give it a shot |
@shaunandrews I just pushed up a change. It looks like it's working for me 😊 . Would you be able to let me know? (Also. I'll fix up that merge conflict) |
That's cause you did it a way better way than my failed attempt. Its working as expected now. -- Something changed with the |
be9908c
to
69825e4
Compare
Ah yes, my bad! I broke the HTML element reference. I just pushed that fix, along with rebase with |
Hey @shaunandrews and others can we get an update to how this PR is coming along. Can we look at the possibility of getting this and associated issue/s into WordPress 5.5? Have a great weekend! |
Hmm! Looks like it has conflicts again. Will help resolve |
I want to sit on this one a bit more before we commit to adding it in time for 5.5. The main issue I see is in the case we are looking for the W menu to open page list / template list in the context of FSE or the page editor, and how it might end up being disruptive if we change the behaviour version to version. |
Closing this for now. |
Fixes #22178
This is a second attempt; The first (and a lot more background info) is #21121.