Fix submenu layout in navigation page list. #34342
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
requested review from
jasmussen,
gwwar,
talldan and
draganescu
tellthemachines
added
[Block] Navigation
|
Size Change: -2 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
jasmussen
approved these changes
Aug 27, 2021
There was a problem hiding this comment.
Nice one, thank you. I'm seeing this in trunk:
After a save and reload, things are fine.
With this branch, things look right from the start, both in editor and published:
Thank you for the fix. Green checks from me based on this working so well, but would encourage a code review beyond this.
gwwar
approved these changes
Aug 27, 2021
There was a problem hiding this comment.
This tests well for me, I also verified that things updated properly when moving blocks around. Feel free to 🚢
Overall, the need for the isNavigationChild attribute is interesting and something to keep an eye on. With this property set, it's possible for us to serialize this in an incorrect way (say that the page list is a navigation child when it's not). A simple way of doing this would be to edit the post in the code editor with the following line:
<!-- wp:page-list {"isNavigationChild":true} /-->The render callback probably doesn't have any information about block parents, so if we end up needing a similar solution elsewhere, it may be worth investigating if it makes sense to provide additional information, or explore an alternative.
Oooh I never thought of that! You'd have to save it in the code editor and exit immediately though, because switching back to the visual editor will change the value to
We discussed passing the actual classnames down instead, but due to our use of |
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
With the classname changes in #33918, the
isNavigationChildattribute was added to the Page List block, so it can render with the navigation-specific classnames when necessary. But because the attribute is being updated inside auseEffect, it isn't present on first render ofPageListEdit, resulting in Page List blocks not getting the right classnames when first added to the Navigation block.This PR fixes the issue by updating
isNavigationChildbefore the component first renders.How has this been tested?
Create a few pages on your test site, and set a few of them to be children of another page.
Add a new navigation block to a post, and choose "Add all pages".
Add another navigation block, choose "Start empty", and then add a Page List block inside it.
Verify that in both cases, the Page List renders child pages correctly as submenus of their parents.
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).