Enable navigation nesting to be filtered and manually set #38621
Conversation
|
Size Change: +90 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
draganescu
added
[Block] Navigation
scruffian
force-pushed
the
add/nesting-filter-for-nav-link
branch
from
f21d0f4
to
037cf41
Compare
|
It seems the testing instructions don't really work, I had my plugin activated and I thought they do. But no matter how many times I tried adding a filter via the console failed for me. Yet, I have the same snippet in a small JS plugin and it works. |
|
In other news, having a conversation with @scruffian about this PR, we found that allowed nesting level is less of a site setting (we don't have such a thing now, except for a technical limitation afaik) but more of a theme setting: usually there is a specific visual menu structure that is broken on deeper than X levels of nesting. Therefore, I will move this PR to make the
|
Currently block attributes can't be set in theme.json. See #35114 |
|
@scruffian yes, I assume though we will want that. |
|
With the |
draganescu
force-pushed
the
add/nesting-filter-for-nav-link
branch
from
037cf41
to
a7b9e41
Compare
|
@scruffian I've updated the PR's description to reflect the new direction. |
draganescu
changed the title
There was a problem hiding this comment.
Thank you for tackling this problem. Seems like it's more complex than anyone initially suspected.
I found one thing - if you have a heavily nested Classic Menu then it will happily import it and not restrict the UI.
Screen.Capture.on.2022-03-07.at.15-00-34.mov
Also as this is important to Theme resilience then I'd like to see an e2e test which covers the max nesting. It just has to follow the same steps as you've described in your testing process for this PR.
|
@getdave that is a great catch. I'll see how to enforce the nesting limit. |
|
@getdave I updated Also I see that |
There was a problem hiding this comment.
Thanks for your persistence with this.
I tested with my deeply nested menu and saw that the nesting level seemed to go to maxNestingLevel + 1.
I then tried adding the nesting attribute to the block manually after import and the menu was still nested at 6 rather than the 3 that I set in the attribute.
Now...I understand why this is happening, but if - for example - this attribute was set by a user or by the Theme on theme switch when you already have a menu in place then it's not going to work.
Scenario: Whilst on Theme A I import a heavily nested menu and it's clipped at 5 levels deep. I then switch to Theme B which restricts Nav depth to 3 levels deep in its Header. After Theme switch, I select my previous menu and it will show at 5 levels deep because there's nothing stopping me right?
I think perhaps we would need to look at this from a different angle. Instead of trying to restrict depth at creation time (i.e. via UI or import) we want to try and do it from the Nav block itself.
Ideally the block would check the maxNestingLevel of it's child nodes and then restrict the visibility of these nodes.
Now I've tried something like this with innerblocks before and I could get it to work. After all, you can't say "here's the list of blocks, but don't render these ones". Therefore I think we'd need to look at a CSS solution for hiding the blocks which exceed the depth.
There are a tonne of tools and selectors for accessing blocks in the tree so this should be possible?
|
This will also need a rebase to account for #39219. I'm sorry 😞 |
|
Yes all valid observations. We should probably need both things:
|
This is important. You might import a menu in Theme A which has maxNesting of I think if we expose a UI toggle for |
|
I will reset this PR to the state where the block offers via context the creation limit and it then goes to display deeper nested menus anyway. It's a good 1st step and we can carry on with the updates to innerBlocks via a follow up PR. |
|
Searching for a cause to the bug with the not working submenu button I compared to |
|
The latest commit fixes the issues I was having |
draganescu
force-pushed
the
add/nesting-filter-for-nav-link
branch
from
8d45d1f
to
6416d72
Compare
|
@draganescu Are you ready for another review here? |
draganescu
force-pushed
the
add/nesting-filter-for-nav-link
branch
from
6416d72
to
d26d608
Compare
draganescu
force-pushed
the
add/nesting-filter-for-nav-link
branch
2 times, most recently
from
a0f0d9b
to
62ddd36
Compare
|
@getdave let's give this a new run, it's basically only about limiting for creation (so it should do nothing to imported classic menus). I added a test. |
draganescu
force-pushed
the
add/nesting-filter-for-nav-link
branch
from
62ddd36
to
51a2a9a
Compare
|
E2E failures feel like flaky tests, I just restarted them. |
| @@ -52,6 +52,7 @@ | |||
| "fontSize", | |||
| "customFontSize", | |||
| "showSubmenuIcon", | |||
| "maxNestingLevel", | |||
There was a problem hiding this comment.
There's one last problem I noticed – the default maxNestingLevel says 5, but I can only go 4 levels deep:
Other than that I think we're good so I'm approving. Feel free to fix it here, in a follow up PR, or push back that 5 is not inclusive, but there's no need to block approvals on this issue.
I got confused and tested the wrong Nav block out of the two I added :D we're good here
There was a problem hiding this comment.
Test both with and without the maxNestingLevel attribute set on the block. Worked as described.
With the test coverage this is a lot better.
My main problem is still that as a Themer I cannot force my Nav to only have X levels of nesting. As discussed we can still import a menu and it will ignore the max nesting setting.
Given that this is likely to be a common workflow it's probably the major weakness of the feature as it stands.
That said, we should ship and iterate!
Thanks for persisting with this one 👏
| @@ -351,7 +349,7 @@ export default function NavigationSubmenuEdit( { | |||
| return { | |||
| isAtMaxNesting: | |||
| getBlockParentsByBlockName( clientId, name ).length >= | |||
| MAX_NESTING, | |||
| maxNestingLevel, | |||
There was a problem hiding this comment.
Should we add this to the dependency array for the useSelect?
There was a problem hiding this comment.
Not sure ... This does not change dynamically ... I see a future where this will have an inspector control for when building themes, but until then it's a value from the markup.
Closes #34611 by making
maxNestingLevelavailable viacontextfrom the Navigation block to the NavigationSubmenu and NavigationLink blocks.To test:
"ref":: this is REFID below<!-- wp:navigation {"ref":REFID} /-->with
<!-- wp:navigation {"ref":REFID, "maxNestingLevel": 3} /-->This update makes the nesting depth configurable by themes. It is also configurable via plugins using the
blocks.getBlockAttributesfilter.