Make SiteHub available in mobile viewports #63060
Conversation
oandregal
changed the title
|
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. |
oandregal
added
[Type] Bug
oandregal
requested review from
youknowriad,
mtias,
jameskoster,
jasmussen,
ramonjd,
andrewserong and
t-hamano
|
Note how the Patterns page behaved in WordPress 6.5. This PR essentially brings back the same behavior: Gravacao.do.ecra.2024-07-02.as.14.23.21.mov |
|
Size Change: +4.47 kB (+0.26%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
Behavior in Gravacao.do.ecra.2024-07-02.as.16.46.24.movBehavior in this PR: Gravacao.do.ecra.2024-07-02.as.16.45.05.mov |
|
I think the main issue here, is that clicking the site hub shouldn't go to the "dashboard" unless you're in the root level, it should instead navigate back. |
|
Alternatively, we could have an extra back button in the hub for mobile? |
|
Would it be unfeasible for the site icon to behave differently on mobile?
That would be more closely aligned to the proposal in #60847. |
annezazu
added
the
Backport to WP 6.7 Beta/RC
|
Adding backport label as ideally these fixes could be included in 6.6. If that's incorrect, feel free to remove. |
|
Having the search/command centre available is great, thanks!
Not sure how to display this, but I agree it would be helpful to be able to return to the main menu. In mobile view, could the site title's arrow icon be displayed at all times (and not just on hover/focus) to indicate that it'll open a new tab? It's a bit of a mystery what the various items do unless you click around. |
I like that idea, too. In order to implement it, would we need to move the sidebar navigation context further up the hierarchy so that it can be accessed from the That said, IMO the current state of this PR is an improvement over trunk in that it now makes it possible to back out again from the template screen, so I wouldn't be opposed to the idea of landing this PR and then we could see if we could add more smarts to the W icon button in follow-ups if need be? |
oandregal
force-pushed
the
try/sitehub-mobile
branch
from
b656fca
to
a0b5c93
Compare
| ref={ toggleRef } | ||
| isTransparent={ isResizableFrameOversized } | ||
| /> | ||
| <SidebarContent routeKey={ routeKey }> |
There was a problem hiding this comment.
@jsnajdr if you have any thoughts here.
There was a problem hiding this comment.
Another side effect of wrapping in SidebarContent is that the site hub receives extra padding, so there's a jump horizontally when we dig into the lower levels of the hierarchy:
2024-07-04.12.04.19.mp4
Edit: After re-checking out this branch I can't reproduce the above 🤔
I've pushed some changes to make it work like this. Gravacao.do.ecra.2024-07-03.as.13.10.12.mov |
| const isMobileViewport = useViewportMatch( 'medium', '<' ); | ||
| const history = useHistory(); | ||
| const { navigate } = useContext( SidebarNavigationContext ); | ||
| const isRoot = |
There was a problem hiding this comment.
Is there a better way to detect we are in the root?
This relies on the search being empty, which it always is at the root level, as far as I've tested. If there's some pollution (a postType=X remanent in the URL, for example) this wouldn't work (but also: we should clean that).
The sidebar uses a explicit isRoot property. Given this is a temporary thing for 6.6 and will be improved by the plans/direction at #60847 I didn't want to modify the public API of SiteHub.
|
That's working well @oandregal. I appreciate I am likely asking for the moon on a stick here, but how about restricting this behavior to when the content frame is visible (when you drill in to Pages, Templates, or Patterns)? On the other pages (Styles and Navigation) the Probably fine to work on that separately if it's not trivial. I'm to get more thoughts on the behavior here from @WordPress/gutenberg-design. |
| <SidebarContent routeKey={ routeKey }> | ||
| <SiteHub |
There was a problem hiding this comment.
Unfortunately it looks like moving the SiteHub to be a child of SidebarContent means that the side bar content's focusing behaviour now matches against the buttons within the site hub, which causes one of the e2e tests to fail ('Can use keyboard to navigate the site editor').
From testing manually it seems that what's happening is that if you navigate to the Pages button from the root and hit Enter, when you're taken to the pages screen, the W icon is focused instead of the back button. I.e. here's where focus now lands:
Here's the function that handles the focusing:
I'm wondering if we need to either have a way for that focusing to skip the site hub and just look at the sidebar area, or if we need a way to give the site hub access to the navigation context without the other wrapping logic?
There was a problem hiding this comment.
I've pushed a commit in 4401f60 that hopefully addresses the focus issue 🤞. It's a little hacky but let's see if it gets the test passing.
|
This is testing pretty well for me, too @oandregal. I've pushed a small (but potentially hacky) fix in 4401f60 to try to get the focusing behaviour in the sidebar to skip the site hub if present. 🤞 that gets the keyboard behaviour and tests passing again. If it feels too hacky, though, do feel free to revert that commit! I also explored a slightly more complex alternative over in #63109, which looked at lifting up where we place the sidebar navigation context, but that wound up causing issues with the back/forward animation and the site hub, so I've closed that one out. |
|
Add me to the list of people this is testing well for too 🙂 @andrewserong's tweak does appear to have sorted out the failing keyboard navigation e2e and works with manual testing. My only question is, do we need to set focus to the W icon button on the pages that don't have the back button? Screen.Recording.2024-07-04.at.3.01.34.PM.mp4 |
|
I've prepared an alternative PR that I like more at #63118 It ticks all the boxes (this one doesn't):
And it doesn't suffer from the technical issues raised: route leaked to SiteHub for behavior control and component leaked to Sidebar for focus control. |
|
Closing this out now that #63118 has landed. Thanks everyone! |
ellatrix
removed
the
Backport to WP 6.7 Beta/RC
Related to #60847 and #62878
What?
This makes the SiteHub always available for mobile viewports, for the Pages, Patterns, and Templates pages. It mimics what we had in 6.5.
trunkWhy?
This brings back what we had in 6.5 for the Patterns page. While the overall direction is #60847 we can me this quick change for improved navigation.
How?
Render the
SiteHubcomponent on mobile viewports.Testing Instructions
Navigate to Pages, Patterns, and Templates and verify the SiteHub is present.