Fix navigation block placeholder preview markup #33963
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
…en a vertical block is selected
talldan
added
[Type] Bug
|
Size Change: +87 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
tellthemachines
approved these changes
Aug 10, 2021
| display: none; | ||
| } | ||
|
|
||
| // Hide entirely when the placeholder can still be toggled. | ||
| .wp-block-navigation.is-selected .is-small & { |
There was a problem hiding this comment.
When is this is-small class added? Having display: none on the placeholder when the block is selected, for either horizontal or vertical, doesn't seem to make any difference visually, so I must be missing something here 😅
There was a problem hiding this comment.
Thanks for reviewing. I can't see what this does either 🤔
Maybe it can be removed.
|
I'm just getting the kids ready for school, but I think the code was there to ensure varying heights of navigation styles were accommodated, not just the big ones from TT1. The way to test is to compare an empty menu with a filled out menu in height in Empty Theme. I'll take a look when I get a moment. |
|
Be mindful of optimizing for TwentyTwentyOne as it has a buggy custom implementation of nav block paddings and margins: https://core.trac.wordpress.org/ticket/53220 |
|
Gave it a shot, all's still working as it should. Thanks for the PR, thanks for the fix! On a separate note, code like what exists for this placeholder has shown its limitations, and has been taken as far as it can with 5.8. In light of #32373, I think there's an opportunity to revisit it and simplify it drastically, perhaps even as much as this:
The idea being, the gray blobs are still perceived as a loading state, and patterns will likely need to step beyond the "start empty" state anyway, and actually insert menu items that are in a setup state instead. Such a refactor would also help untangle all the gray-blob-edgecases as evident by this PR. |
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 two separate issues with the navigation block placeholder preview.
First, the preview has invalid html, nesting an
svgelement in aulelement:Secondly, when using a vertical navigation block, the invisible placeholder was taking up height, leading to this situation after selecting 'Start empty':
How has this been tested?
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).