Navigation: Add clearance for appender in submenus. #36720
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
|
Size Change: +76 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
jasmussen
added
the
Backport to WP 6.7 Beta/RC
priethor
approved these changes
Nov 23, 2021
There was a problem hiding this comment.
Thanks @jasmussen ! Works as shown in the demo above, and I didn't find any strange behaviour when building complex submenus.
|
Thank you Hector! Per some work in #36656, I'm tempted to get a second look by @youknowriad, I suspect this might stop working when we land that one. |
|
Indeed, this will stop working when that PR lands, we should try to leverage the |
|
I tested this branch with Screen.Recording.2021-11-23.at.3.32.23.PM.movThis seems to work fine with those changes, but I can reconfirm when that gets merged to trunk. I see the appender appear when the parent is selected, and it does not overlap with the right caret in the menu. Is it OK that the menu changes size to accommodate the appender when the parent is selected? |
|
Thank you for testing! Looks like the behavior of the clearance is definitely good. There's an additional nuance that I'd love to add, which is to make sure that the plus button shows up even when an adjacent menu item is selected. Essentially reversing the change from #36656 but just for the submenu block. Here's a GIF that hopefully explains it better: In this GIF you see me clicking a submenu block to see the plus inside the open submenu. I then click a menu item inside, at which point the submenu is now a parent that's selected, and it still shows the plus. The thing here is that for building submenus it doesn't feel the most intuitive to have to select the submenu again when you want to add an additional item. Edit: Oh I guess rereading it we're all, already, on the same page. Sorry about that!
Yes, I think for this particular use case it's an exception to the rule which we can allow because it helps the user experience, while at the same time not causing layout shifts. |
7 tasks
georgeh
force-pushed
the
update/navigation-appender-padding
branch
from
0838ca2 to
e811fba
Compare
|
Thank you for the help @georgeh! I pushed tweaks so it works like this: The appender is now the white plus, and takes up the full space that submenu item will take. I think it works well. I did briefly ponder whether we need to hide the appender when a nested child is selected (i.e. if you're 3 levels deep, only show the appender for the immediate parent), but I'd like to try and start with the most basic behavior. In this case, I really appreciate the simplicity of this fix. What do you think? @youknowriad not super urgent, but if you can just glance at it I'd appreciate it. |
jasmussen
force-pushed
the
update/navigation-appender-padding
branch
from
12205ca to
387e02e
Compare
|
I'm going to land this one since it's so small, and I'll be here for any followups. |
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
noisysocks
pushed a commit
that referenced
this pull request
Dec 13, 2021
* Navigation: Add clearance for appender in submenus. * Trying ButtonBlockAppender instead of DefaultBlockAppender * Tweak the behavior. Co-authored-by: George Hotelling <[email protected]>
7 tasks
8 tasks
82 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.
Description
Followup to #36605, which was important for avoiding layout shifts, but should be considered a step on the way to greater things. This PR fixes what I'd describe as an edgecase for the navigation block, where building submenus benefitted from the previous behavior, and the abs positioned behavior becomes a bit clunky:
This PR adds an edgecase explicitly for the navigation block:
The CSS works fine for this, but the particular use case, building vertically expanding lists, is something we should consider when we explore the next iteration of the appender.
How has this been tested?
Build a submenu with the navigation block.
Checklist:
*.native.jsfiles for terms that need renaming or removal).