Navigation: Fix click-button size, submenu directions, scrollbars. #36215
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
jasmussen
commented
Nov 4, 2021
| @@ -130,6 +130,7 @@ | |||
| // Don't take up space when the menu is collapsed. | |||
| width: 0; | |||
| height: 0; | |||
| overflow: hidden; // Overflow is necessary to set, otherwise submenu items will take up space. | |||
There was a problem hiding this comment.
This overflow rule I believe I had in the past, and then removed it. In this PR it appears to work as intended, with no ill side effects. But I would appreciate thorough testing of submenus, with and without click, and with and without keyboard usage. Just to make sure I'm not missing anything.
|
Size Change: +91 B (0%) Total Size: 1.09 MB
ℹ️ View Unchanged
|
vcanales
reviewed
Nov 4, 2021
There was a problem hiding this comment.
The right alignment fix works but only when explicitly aligned right; it doesn't seem to fix the problem when the navigation menu is used in a template, where it happens to sit at the right of the screen.
It's also noticeable that long submenu items might overflow outside of the viewport; is it possible to wrap them conditionally? ie. when they exceed the viewport.
Other than that, I didn't find any other details! 👍
|
Thanks so much for the thorough review! I'll look at addressing this. |
7 tasks
Yes, that's a bit of a limitation of how things work at the moment, but beyond the scope of this PR to fix. I created #36241 to track this further. |
It appears I intentionally added nowrap to menu items in #35077. I'll see if I can remove it again, but I think it was necessary due to some scaling issues. It also surfaced some other issues, which I'll look into. |
|
Fixed two issues. First off, the explicit application of a z-index when the mobile responsive menu is enabled caused this: Setting it to "auto" fixed it: I also removed the |
|
Alright, fixed a last issue with justificaitons broken when mobile responsiveness was enabled.
This one should be good to go now. |
|
This one needs a good rebase after the merging #36169. |
By itself, it's looking good to me. Rebase-wise, there was only one conflict to fix: .wp-block-navigation__responsive-container-content {
<<<<<<< HEAD
display: contents;
=======
display: flex;
>>>>>>> 329693fea1 (Fix issue with justification and responsive container.)
}
|
vcanales
force-pushed
the
fix/submenu-scrollbars
branch
from
329693f to
c389fb8
Compare
|
Thank you for the rebase help. It might need more changes, though, as 36169 removed the justification classes, so menus no longer open in the correct direction at all. I think @tellthemachines had some ideas to explore here as well. |
jasmussen
added
[Status] Blocked
|
I've extracted some of the small fixes (line height, overflow etc) into #36298, which can likely land easily as it's small. This PR should rebase right over it, but is still blocked on either restoring the justification classes or finding another way forward. But by extracting the other things, this one can focus up a bit. |
jasmussen
force-pushed
the
fix/submenu-scrollbars
branch
from
c389fb8 to
b5f5a5c
Compare
|
Alright, I think this one is ready for review again. A lot of what this PR originally fixed has now been made moot as it was extracted and fixed separately, so there are primarily two things left to test, and both in my own testing appear fixed by this PR.
|
jasmussen
added
Backport to WP 6.7 Beta/RC
|
@jasmussen I can no longer reproduce either of those issues on trunk. To clarify, were the submenus on small breakpoints not opening to the bottom? I've been comparing trunk to this branch and not quite sure what has changed 😅 |
|
Sorry the editing of the original issue actually removed helpful instructions. I still see this happening on trunk:
That's becasue the rules for justifying the menus are wrapped in a media query. So the steps to test:
At around that breakpoint, the menus should in this PR start to open downwards, but still be right aligned to their parent item. In trunk, they just open rightwards at that point. |
|
Oooh got it, thanks. I was looking at the nested submenus and ignoring where they were sitting at the top level 🤦 I think this is good to go! |
tellthemachines
approved these changes
Nov 12, 2021
|
Thank you! |
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
noisysocks
pushed a commit
that referenced
this pull request
Nov 15, 2021
…36215) * Navigation: Fix click-button size, submenu directions, scrollbars. * Fix wrapping, fix z index issue. * Fix issue with justification and responsive container. * Remove duplicate rule.
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
Fixes WordPress/twentytwentytwo#206. As reported there, there are a few issues with dropdown submenus and their nested versions as well:
- Even when the submenu is closed, its contents take up space and can in some circumstances create a horizontal scrollbar.- Not reported in that ticket, but surfaced by it — menus set to open on click had smaller buttons for the submenus than for hover.Some of the bugs originally fixed by this PR have been fixed separately. But the two remaining issues are still good to land.
How has this been tested?
- Create navigation menus that features submenus, and sub sub menus (nested menus). - Test with and without click enabled. - Test a justified-right submenu on small (<782) and bigger breakpoints. They should both open leftwards, but <782 menus should open leftwards and _downwards_ (as shown in the GIF above). - Bonus: use the web inspector to verify that submenus are zero width even when not shown.Note that we usevisibility: hidden:to hide submenus so we can nicely fade them in and out.display: none;is an alternative that won't let us animate it as nicely.Test instructions and GIFs are shown in #36215 (comment).
Checklist:
*.native.jsfiles for terms that need renaming or removal).