Navigation: browse mode list all Navigation Menus. #50840
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
getdave
added
the
[Feature] Navigation in Site View
getdave
force-pushed
the
add/nav-browse-mode-list-all-menus
branch
from
413b850
to
598b943
Compare
getdave
force-pushed
the
add/nav-browse-mode-list-all-menus
branch
from
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. |
scruffian
reviewed
May 22, 2023
| ) { | ||
| history.push( { | ||
| postType: attributes.type, | ||
| postId: attributes.id, |
There was a problem hiding this comment.
What do you think about using slug instead of ID? Is that possible?
There was a problem hiding this comment.
Possibly. In this PR though?
There was a problem hiding this comment.
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.
I'm going to look at the routing again and see what we have available.
There was a problem hiding this comment.
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. |
MaggieCabrera
approved these changes
May 23, 2023
No, happy for this to be merged and then follow up in future PRs with more changes :) |
cbravobernal
changed the title
youknowriad
reviewed
Jun 15, 2023
|
|
||
| // 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.
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.
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.
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
16 tasks
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?
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
Navigationbutton 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.
Navigationin Browse Mode and click it.Navigationin Browse Mode.Navigationoption.Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2023-05-22.at.13-49-57.mp4