-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Try: Fix so submenus only take up space when visible. #34382
Merged
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: +112 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
vcanales
approved these changes
Aug 30, 2021
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.
Tested, and LGTM!
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
[Block] Navigation
Affects the Navigation Block
[Type] Bug
An existing feature does not function as intended
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
At the moment, navigation block dropdown menus are hidden using
visibility: hidden;
. This enables us to apply CSS animation, the alpha fade, when the menu becomes visible. The problem is that unlikedisplay: none;
, simply hiding an element using visibility doesn't remove it from the DOM, it's still there taking up space:Specifically when you have dropdown menus that nest deeply, this might cause a horizontal scrollbar to appear, even when the menu is supposed to be invisible. I'll come back to the horizontal scrollbar in a moment.
This PR fixes it so that when a submenu is meant to be invisible, it also doesn't take up space in the DOM, causing any scrollbars at all, at least until you actually open those submenus whether through hover or focus:
About the scrollbar. Horizontal scrollbars are not desirable. At the moment the navigation block doesn't have any built-in smarts to reverse the direction of submenus when bumping against screen edges, and until we've reached a more thorough point of stability with the block itself, it's probably not advisable to spend too much time making that happen. However there are multiple tools at a users disposal to mitigate and/or address such items. For example, if you justify right, menus open towards the left. If you justify space-between, the last item opens leftwards.
So for now, this PR is not about avoiding a scrollbat per se — just to ensure that if it shows up, it shows up in context of the submenu that is causing it.
How has this been tested?
Create a menu with lots of nested submenus. Test that they appear the same as before. Please test menus with placeholder items nested in the editor as well.
Checklist:
*.native.js
files for terms that need renaming or removal).