-
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
Navigation: browse mode list all Navigation Menus. #50840
Conversation
413b850
to
598b943
Compare
598b943
to
ef90205
Compare
Size Change: +440 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
Flaky tests detected in 84c4e94. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5045996147
|
This looks like a good start. I think that when you select a navigation we should update the preview to show only that navigation, but we can do that in #50579. |
) { | ||
history.push( { | ||
postType: attributes.type, | ||
postId: attributes.id, |
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.
What do you think about using slug instead of ID? Is that possible?
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.
Possibly. In this PR though?
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.
Happy for it to be a followup, but remember once people start using these permalinks we have to support them so it would be good to look into it before the next Gutenberg release.
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'm going to look at the routing again and see what we have available.
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.
Thinking about this more, since pages already use IDs, I think it's fine for navigations to do the same
@scruffian Thanks for the review. What makes you think this isn't ready to merge already? Is there anything you noticed that would block that? |
Nice one. Instead of saying "Loading navigation menu." can it just show a spinner? In general whenever something is loading, we should either show nothing (assuming the loading is fast), the spinner, or even the spinner after a small delay in case it finishes loading before that. The left spacing of the nav menu here is a little off. I'm assuming that's the list view leaving room for nested chevrons? We might want to massage that, can happen separately. I know @jameskoster has some detail page designs for this. I would echo Ben, that when you select an individual menu to load, we need to load in the preview just that one menu. Whether that happens here or later, I'll defer to you. |
Thanks for the review @jasmussen.
Yes we can change that. Noted as a pattern for future reference.
Yeh I also noted that and you're right it is due to the need to maintain space for chevrons. Happy to receive additional input on this design-wise and tackle in a followup.
Agreed. We'll have to do it in a follow up because we currently don't have any way to load an individual menu in focus mode until we actually do the work to implement a focus mode 😅 I'm planning to look at this next. Update: also I've fixed things so that the menus query is now always preloaded which means you rarely see the spinner. There is an unrelated bug which causes a spinner for the single menu view but that's something for a followup. |
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 testing very nicely and looks solid as far as my understanding goes
No, happy for this to be merged and then follow up in future PRs with more changes :) |
|
||
// If there is a single menu then we can go directly to that menu. | ||
// Otherwise we go to the navigation listing screen. | ||
const path = hasSingleNavigationMenu |
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'm not sure it's wise to do this (change the path depending on the number of menus). This creates an issue because now the "parent" of the navigation is "hidden". so for instance if you go to "navigation" and you click "back" you don't end up in the root place.
I wonder if the best solution is to instead of doing this, change the behavior within the /navigation
screen to actually render the menu directly when there's one.
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.
Also, another unwanted side effect of this is that in any place where we want to "navigate" to the "navigation" screen, we need to make the same check over and over again (check if there's a single menu and change the destination path). For instance I'm working on the command center and there's no way for me to add a link to "navigation" by just setting a path, I need to duplicate this code there too.
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.
That's a good point. I don't want to expose the need to duplicate that logic. Thank you for spotting it.
I think I had that functionality initially but the issue was that I needed a dedicated route for a single Navigation. That is a requirement for Nav Focus mode.
That said, if we accept the above, then we could also make the navigation listing route conditionally display
- the list of menus
- a single menu if there's only one
As long as we still retain the single route that would be good.
I'll raise an Issue
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.
What?
Adds a list of all Navigation menus to the Browse Mode section of the Site Editor.
Obeys rule:
All menus are accessible directly via URL as is the listing screen.
Closes #50578
Also addresses part of #50579.
Why?
Address the part of the UX and Design shown in #50578. This part of the wider effort to add Navigation to the Browse mode.
How?
Creates dedicated components for listing all menus and showing a single menu.
Updates the main Browse Mode sidebar to use a special component for the
Navigation
button which will route according to the number of Navigation menus it finds (obeying the rules set out above).Known Issues
Testing Instructions
Throughout the testing below keep clicking back button and checking it routes you as expected.
Navigation
in Browse Mode and click it.Navigation
in Browse Mode.Navigation
option.Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2023-05-22.at.13-49-57.mp4