Fix submenu justification and spacer orientation. #36340
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
tellthemachines
added
[Block] Navigation
tellthemachines
added
the
Backport to WP 6.7 Beta/RC
|
Size Change: +503 B (0%) Total Size: 1.09 MB
ℹ️ View Unchanged
|
|
Really nice, thank you. This fixes it in the editor right off the bat: It doesn't appear to be working on the frontend:
I'll dive in and see if I can push a fix for that. |
About the placeholder, that's a good question, and goes to the heart of how we should design placeholders. The thing is, in a site editing context, the placeholder should ideally have a footprint that is identical to that of the final result. Without it, it will cause layout shifts. There's wiggleroom of course — there are some blocks where the pre-configuraiton of key values is so crucial that it deserves up-front weight, but even then we have to consider the context in which they will be used, such as small viewports, or of the block is inserted inside a thin column. For example the navigation block in a column is already leveraging the built-in
As far as block placeholders that look like the end result, my current favorite is the Site Logo: Its footprint is identical, and it inherit colors and styles from the theme even in its unconfigured state. So back to your question, can we remove the |
Thanks for working on this. I'm not sure what to look for, as trunk and this branch appear the same to me: Some issues:
It also made me realize that the mover controls are now all vertical, for all blocks inside the container, despite having horizontal orientation. This appears to be a general challenge for flex orientation blocks, as the Buttons block also shows vertical movers when it should be showing horizontal ones:
Do you know if this is tracked separately? In any case, since these are general issues for the spacer and mover controls, they should not block this PR from fixing the justifications. |
|
Back to the justifications which work in the editor but not in the frontend. It appears the classnames are working perfectly, the missing piece is that the
I suspect that's beyond my skill to fix. But it seems like once we add those classes, we can land this one. Thanks again! |
|
Ok I've fixed the justification classes on the front end, and the Spacer issue (there was no default value for orientation; I added one in so it should always work now). Also removed the |
tellthemachines
force-pushed
the
fix/submenu-alignment
branch
from
32d14a8
to
e2b0c54
Compare
|
I'm having some local development environments that make me unable to test latest trunk, or this PR, at the moment. But if this works on the frontend, it'd be good to land, we can always follow up on things. It would be good with a quick sanity check by another. |
jasmussen
approved these changes
Nov 11, 2021
There was a problem hiding this comment.
Works well:
Thank you for the cleanup as well. As noted, I think I might be able to restore some of the vertical behavior for the placeholder when the orientation is changed — whether that's desirable remains to be seen, but no reason to lug the cruft along with it in the mean time.
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
noisysocks
pushed a commit
that referenced
this pull request
Nov 15, 2021
* Fix submenu justification and spacer orientation. * Add back justification classes in the front end. * Add default orientation for Spacer. * Remove legacy attributes and styles. * Fix php lint errors. * Mark orientation change as not persistent.
6 tasks
This was referenced Feb 7, 2022
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
#36169 broke submenu alignment when the Navigation block is aligned right or space-between. This PR adds back the missing classnames as a temporary fix until we can sort out a better way to align submenus (#36241)
It also patches the orientation context that Spacer block needs to work correctly inside Navigation. Again, this is a temporary measure until we find a better way to do this (#36197)
In doing this I noticed the Navigation block still has an
is-verticalclassname to which a few editor styles are added, mainly to do with the placeholder. I'm thinking these might no longer be needed since vertical/horizontal is now a setting instead of a block variation, so the placeholder will, in principle, never appear in vertical form. @jasmussen could we remove these styles?How has this been tested?
Add a Navigation block with some nested submenus (at least two levels of nesting).
Align it right, and verify that 2nd level nested submenus are opening to the left instead of to the right.
Add a Spacer block inside the Navigation.
Verify that in horizontal orientation, the Spacer grows horizontally.
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).