Enter focus mode when single Navigation is edited on "listing" Navigation browse mode route #51795
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
getdave
added
[Type] Bug
getdave
commented
Jun 22, 2023
packages/edit-site/src/components/sidebar-navigation-screen-navigation-menus/index.js
Outdated
Show resolved
Hide resolved
|
Size Change: +2.01 kB (0%) Total Size: 1.45 MB
ℹ️ View Unchanged
|
|
@aaronrobertshaw @glendaviesnz as you've been working in and around the same code have you noticed how flaky the state sync with URL is? We need a solution for the problem this PR solves but we also need to fix the syncing so that when the url changes the state is also updated correctly. That's not currently happening. Do you have any thoughts about how we could improve this? |
|
Thanks for the ping @getdave 👍 This might be something that @kevin940726 could help with. He solved a number of navigation-related issues with the recent library feature e.g. #51558. |
getdave
force-pushed
the
fix/enter-focus-mode-for-single-menu
branch
from
a86a37e
to
2d75a00
Compare
Turns out I was thinking of this backwards. The whole system is set up so that the route is the source of truth. I think that's correct. I was trying to manipulate the state directly and that was getting overriden by the route syncing. So now I'm simply redirecting to the correct route rather than trying to render that [single menu] route's contents on the listing view. I am also avoiding having to land on that listing screen at all if there is only a single Navigation. |
|
Flaky tests detected in 2d75a00. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5388931488
|
kevin940726
approved these changes
Jun 27, 2023
There was a problem hiding this comment.
Nice solution! I found some weird behavior while trying to hit the Back button on the sidebar though. Sometimes the back button doesn't really "go back" in the browser's history stack, it instead pushes a new entry onto the stack. It doesn't necessarily affect users but could be improved if needed.
| // more than one menu. | ||
| useEffect( () => { | ||
| if ( navigationMenus?.length === 1 ) { | ||
| history.push( { |
There was a problem hiding this comment.
We probably want replace here so that it doesn't add an additional stack to the history?
| import SidebarNavigationItem from '../sidebar-navigation-item'; | ||
| import { useLink } from '../routes/link'; | ||
|
|
||
| export default function SidebarNavigationScreenNavigationMenuButton( props ) { |
There was a problem hiding this comment.
This works but we're also moving the fetching logic to the main screen. We'll be executing this as soon as we land on the site editor, the logic is no longer fetched lazily. I think this is okay but probably worth double-checking with someone more experienced with the navigation code (like you!). Maybe we're already doing this or prefetching it too, then it should not be a problem!
There was a problem hiding this comment.
It's a good spot. Luckily this query is already preloaded on the PHP side. So it's safe to preload this data here to determine the correct route.
Other than this solution I can't see another way to enable this kind of selective routing to single/listing 🤷♂️
|
I think the current behaviour is the behaviour we want - see the original issue here: #50396
We should have an edit button by the title which allows you to get into focus mode, but that seems to be missing, so we should follow up with a fix for that. |
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.
What?
Ensures focus mode is entered for a Navigation menu if it's edited on the listing screen of Browse Mode. Fixes issue introduced by #51565.
Why?
In #51565 we made it so that if you go to
Navigationand there is only a single menu then it just renders that directly on that route. This works well, except that you no longer enter focus mode on that menu.That's a bug because on the actual single route you always enter focus mode.
This is because the focus mode relies on the edited post being set in state. With multiple menus this is achieved when you browse to a single menu as the params in the URL are synced to state by a special component and
setNavigationMenuis called with the post ID from the params.However when there is a single menu the state update never happens as the menu's postID is not in the URL which remains as
/navigation.Therefore we must redirect to the route for a single menu.
How?
If there is a single Navigation Menu then:
/navigationthen we redirect to the route for the single menu.Navigationin the root of Browse Mode then you go directly to the route for the single menu.Testing Instructions
NavigationNavigationand then duplicate your menu.Testing Instructions for Keyboard
Screenshots or screencast