Nav block: Fix onselect jump #31198
Closed
Nav block: Fix onselect jump #31198
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
added
[Block] Navigation
|
Size Change: +385 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
getdave
reviewed
Apr 27, 2021
There was a problem hiding this comment.
I tested this and in the specific test case provided it worked as expected.
However, there might be an edge case.
- Create Nav Block.
- Click
Start empty. - Deselect the Nav Block by clicking outside.
- See placeholder state in the Nav Block. Note the height this occupies.
- Click back into the Nav block. Note that the height now changes.
Screen.Capture.on.2021-04-27.at.10-39-17.mp4
Perhaps we need an is-empty-state class which we can use to qualify the is-selected rule to apply the min height on in that state?
Other than that it is good but I wonder perhaps whether the rule you have removed was originally designed to cater for the edge case I described above. If it was then it should have had a code comment to indicate this so if we fix it we should do that.
jasmussen
force-pushed
the
try/nav-select-jump
branch
from
1efd3b4 to
e4c5c18
Compare
|
Excellent steps of reproduction, I had somehow missed that bit. Thanks so much for your time here. Your comments made me think of a way to maybe solve this, so I've pushed two fixes. But for either fix, there will be a small jump, so it's a question of where we want to make the jump. First I pushed a fix to make it like this:
What happens there is that there's an invisible space being output. That space character happens to be the same height of a menu item, which means there's no jump between an empty nav block and one with menu items. I went a step further and removed the minheight entirely in favor of such a paragraph, so that now the gray skeleton indicators are also 1 menu item tall:
In both cases, there's a small jump. I believe this to be inevitable with the current way the navigation menu placeholder state is created, because a blue block editor button will always be the height of a blue block editor icon. I think there's further thought we can do to both the skeleton indicators and the placeholder state, but perhaps this is a good interim fix? |
7 tasks
|
Thanks to all your great reviews, I actually realized some limitations in the approach here. In looking at an adjacent issue, I ended up fixing the same issue there, so I am closing this one in favor of #31371. |
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
The nav block had a min height on select, which caused a jump:
This PR fixes that:
Why was it there? Well, the placeholder state needs it to not collapse. But it doesn't need it in the configured state:
How has this been tested?
Test a navigation block, both empty and populated, horizontal, check that it looks good.
Checklist:
*.native.jsfiles for terms that need renaming or removal).