-
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
Site Editor: Always use full screen mode #29489
Site Editor: Always use full screen mode #29489
Conversation
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @creativecoder! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
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.
These changes look pretty reasonable to me. I wonder if anyone else involved with the Nav Panel might notice something we might have overlooked, but it seems to work well!
Another thought (although potentially as a follow up), is maybe we should be hiding the top-bar that is shown in these smaller screen widths in the site editor:
With the Nav Panel available, this top bar really doesn't bring much to the user. 🤔
Note that I ran into a keyboard focus issue while testing this (not caused by these changes). Opened #29536 for tracking. |
Blocker: gutenberg/packages/edit-site/src/components/header/style.scss Lines 11 to 20 in 10ee7ee
Non-blocker: (follow-up?) Or how about making the sidebar full width below 400-500px? 🤔 |
Closing the sidebar upon selection of a menu item feels loads better to me 🎉
I'd agree with that. We hide the admin bar on desktop view, probably makes sense to hide it on mobile as well. +1 to doing that in a follow-up though, in case there is some context missing as to why it is not currently hidden. |
@david-szabo97 Thanks for catching this! Addressed in 7338f1a by removing the media query, as suggested.
I think that's a good idea. I'd like to push this to a follow-up PR, as it also brings into question some additional concerns... for example if the navigation panel is full screen, the lack of an obvious close button becomes more of an problem. |
Something is going on with the E2E tests... Can you try a rebase? 🙏 Thanks! I just noticed that the block toolbar overlaps the browsing sidebar on medium screen sizes. Let's fix that in a follow-up. |
Thanks for pointing that out, @david-szabo97 ! I tried rebasing twice so far today, but some tests are still failing. All failures are related to the block inserter not being open to insert a block. So far I'm unable to replicate the failure locally, so I'm not sure what's preventing the inserter from opening. |
Ran the E2E tests locally and they are failing. Checked the screenshots as well. What I don't understand is why the browsing sidebar is open? 🤔 After
I think this causes the E2E tests to fail. Can you try reverting this locally and test it? We could also revert it in the PR and do it as a follow-up. |
I can confirm after commenting out these two lines, tests are passing. and |
The inserter button goes behind the W button for a few milliseconds. Puppeteer tries to click on it but it's not visible in the viewport. Instead of clicking the inserter button it clicks on the W button. That's why in the screenshot the browsing sidebar is still open. Opened a PR that will fix the problem above. |
I think we can remove all instances of
|
@david-szabo97 Thanks so much for helping me figure out what's going on with the e2e tests, and for your fix in #29762! There's enough complexity going on with the e2e tests here that I'm going with your idea of using a separate PR: I've removed the change that closes the navigation panel after selecting a template and updated this PR description. I'll open a separate PR with that feature + the associated e2e test changes. |
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.
This is working nicely for me!
Should we be displaying the default wpadminbar
at the top on smaller screens (<782px)? I'm mostly using Customizer here for comparison, which is another editor that forces fullscreen. In that case it's not shown and I'm thinking we should hide it in the case of Site Editor too.
I also noticed that exactly at 782px width we end up in this weird state where admin bar takes up the space but it's not shown:
@vindl I think the answer is no, to be consistent with how full screen mode works. (@Addison-Stavlo made the same suggestion in an earlier comment). I'm tracking that in #29805 |
This is now passing CI and should be ready for a formal review. I've moved the feature to auto-close the sidebar after selection to #29806 |
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.
Approving from the design side :)
Great, I think it makes sense to address this in a follow-up since it's a separate decision. This is looking good to me now! |
Automatically closes the navigation panel within the site editor after selecting a template or content item. Follow-up to #29489: it's particularly helpful on smaller screens, where you would otherwise be left with the navigation panel taking up most of the screen after making a selection.
Description
This change addresses #26125 and #29339 by always using full screen mode in the site editor.
How has this been tested?
Screenshots
Types of changes
Removes a UI setting in the site editor.
Checklist:
I've included developer documentation if appropriate.(n/a)I've updated all React Native files affected by any refactorings/renamings in this PR.(n/a)