Navigation on Browse Mode: Move the action to the leaf menu #50843
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
scruffian
added
[Block] Navigation
3 tasks
|
Size Change: +4 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in ac89ec4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5291720939
|
getdave
reviewed
May 22, 2023
There was a problem hiding this comment.
The change itself seems good. Thank you for working on it 🙇
The only bug I found was when I added a Category link using the Category link variation. It shows the Navigation to... item but when clicked it does nothing.
Conceptually however I think users will find this super confusing. We've been teaching them that in list view "click" means "select this item". Now we're adding an edge case where "click" does nothing at all and you have to know about some obscure option to trigger that action.
Whilst I appreciate the sentiment that it might be strange to have click navigation to a page, I feel what we have in this PR is not a good experience and we should reconsider.
|
The label is a little tricky – "Navigate to X" could be interpreted as viewing the page on the frontend. Perhaps something like "View page details" or even "Edit page details"? Related, but not something to address here... is there a chance "Remove X" could be mistaken for deleting the page? Maybe a more generic "Remove link" would work better? |
|
How about "Go to X"? |
|
I don't know that we need to include the title? It's already clearly visible in the menu item, and we'd need to truncate long titles. Given we removed the block name in the block toolbar's "Delete" action, it may make sense to be consistent here? |
Or "Delete" (re consistency with the block options).
What about "Go to page" or even the classic and familiar "Edit page"? I don't think we need to be prescriptive about the editor view you're in (if page details is open, or you're right in the editor). Editing the page starts with the Page Details view. |
|
I thought "Edit page" initially. What put me off is that you can't actually edit anything in the details view yet, and there is a separate "Edit" button there too which makes things a little awkward. There may be an argument for separate actions in this menu:
That's probably a bigger discussion though. For now I'd lean towards "Go to page" which simply opens the details panel. |
13 tasks
scruffian
force-pushed
the
move/navigate-to-submenu
branch
from
c20bb02
to
ac89ec4
Compare
|
I rebased this, so it's ready for another review. |
|
Generally looks like this PR is landable, as the core behavior feels like an upgrade. No chevron to indicate drilldown, and the ellipsis menu shows a "go to": 
However there are some glitches in the site view navigation overall, one of which may be worth fixing in this branch. Notably here: 
The width of the active bar does not feel balanced left and right, and the ellipsis doesn't line up with the heading ellipsis. I understand that the icons on the left are a bit trickier since there has to be room for the chevron for submenus, but I think with a little CSS that can be fiddled as well, so that those left icons are positioned according to the same type of icons in other menus. Compare with icons in the top level menu: 
Almost certainly separate: 
Let's not use the red color for the button here. In general I think we should move away from "destructive colors" as they do not work well for color blindness and don't always culturally have the correct significance. In this case the confirm dialog itself is enough validation that you are sure. You can also change the "Confirm" label to "Delete". There's also a bit of an issue with how multi menu management works. The heuristic is this:
Note the following GIF how this only partially works:
In this GIF it shows how clicking "Navigation" skips the "Navigation" section and jumps to the "Header navigation" section, so when you click the "Back" button you go to a "Navigation" section you didn't navigate to, showing a single drilldown for the menu that's saved, like so: 
You should never see this. When there's only one menu, show it directly on this top level of the Navigation section, as per #50396:
|
|
Looks like Jay noted some of the alignment issues already in #51585 (comment). |
This has been noted in #51561 and we are fixing in #51565
If that's the case then let's deprecate the props of the relevant |
|
CC: @ciampo in case you have advice on the color bit. |
|
Oh sorry for the double ping, I just now realize you already created a separate issue! You're popular, Marco! |
jasmussen
approved these changes
Jun 20, 2023
|
This PR makes a bug that exists in trunk more visible: 
This can also be seen in |
|
🥳 |
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
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?
This moves the navigate action from being the onSelect action of the block to being hidden under the ellipsis
Why?
This action is only relevant on items that link to a page. It also takes users to the Pages section of browse mode, so it makes more sense as a secondary action.
How?
We no longer need to modify the behaviour of the onSelect action in the List View, so the code becomes simpler - we just move the code to the leaf-more-menu file for this component.
Testing Instructions
Clicking on the ellipsis menu by the page and post link blocks should open the more menu with the option to "Navigate to [Page X]". The custom link block should not provide the option to navigate.
Also try adding blocks without urls/page set and confirm that these blocks don't show the option to "Navigate to X".
Also confirm that the action to select a page and then navigate to it from the List View does not work.