Site Editor: Fix navigation on mobile web #59014
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +62 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 9ca090e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7914592211
|
t-hamano
reviewed
Feb 15, 2024
There was a problem hiding this comment.
Thanks for the PR!
I hid "manage all pages" like we do for templates and template parts
For patterns, I remove the sidebar filtering because I didn't want to introduce specific implementations in the layout for a given page.
At this point, I also agree with this decision. In the future, as reported in #49769, I would be happy if we could also publish the "Manage all XXX" menu in the mobile viewport.
I tested this PR, and when I click on the pattern menu, the pattern category appears only for a moment, and then the Dataview appears.
64b6ae722e7f73ea092469f85a4cf8a1.mp4
When that becomes default, we'll have to address it indeed. But I think the solution for it is probably to use the "canvas" in the URL for mobile. |
I pushed a commit that make this less visible (basically shows nothing for a millisecond). This can't be solved entirely until we actually make the "sidebar" part of the routing (get rid of the Navigator component or make it controlled). It's the result of the synchronization between the internal path of the "navigator" and the url. so there's a small delay when the two are not synced which gives this result. |
ntsekouras
reviewed
Feb 15, 2024
| </AnimatePresence> | ||
| </NavigableRegion> | ||
| { ( ! isMobileViewport || | ||
| ( isMobileViewport && ! areas.mobile ) ) && ( |
There was a problem hiding this comment.
Based on the above comment, shouldn't we render NavigableRegion?
|
When I click the Edit button on the Styles page, the canvas view is not displayed. This problem does not occur with trunk. Maybe we need to adjust the conditional somewhere. 281cb1bb4d2afcbfd9f262eada607202.mp4 |
ntsekouras
approved these changes
Feb 15, 2024
There was a problem hiding this comment.
Thanks for the PR Riad! Besides my comment about NavigableRegion, this looks good.
This is a tough one and while not perfect, I think it's pros outweight the cons. I had also explored this a bit by adding a sidebar area instead of your mobile area, but thought it added way more complexity than your suggestion.
I'd say this is an improvement right now for mobile and when we implement the sidebar as part of the routing, we'll have a much improved experience.
|
I don't think we need a region because the regions are defined by the |
|
Thanks for the update! I confirmed that the canvas view is displayed when I click the Edit button on the Styles page. This code is a bit complicated for me, but I agree that this PR is a solid improvement for the moment. |
youknowriad
added a commit
that referenced
this pull request
Feb 20, 2024
Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org>
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
added
the
Backported to WP Core
youknowriad
added a commit
that referenced
this pull request
Feb 20, 2024
Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #58044
What?
This address the biggest outstanding issues when it comes to mobile web in the site editor. While also keeping things simple by introducing a new "mobile" area in the router.
This PR makes the assumption that on mobile, there's not multiple areas to render pages like desktop (sidebar, preview, content), there's only a single area, so each URL should define what to render on the mobile area in the router.js.
I've made some decisions here:
Testing Instructions
1- Open the site editor on mobile web (small viewport)
2- Navigate to all the pages and check that everything is correct.