Add generic classnames to children of Navigation #33918
Conversation
tellthemachines
added
[Block] Navigation
|
Size Change: +469 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
This is so beautiful. It also feels like it fully moves the responsibility of handling the spacing, positioning and margins of navigation contents to the navigation block itself, rather than on every block that will sit inside. This feels like a very solid move in the right direction.
Getting page list in as a part of this would be so amazing, and would likely fix #31879. In my testing, this is now working on the frontend, but I don't yet see the classes inside the editor.
I'd be more than happy to help sort that out after this PR!
Good question. Is this something we could do at a later point? Just looking at spacing things out, it could be useful:
However since the markup currently puts menu items inside a menu item container, my instinct would be to wait and see:
Nice work! |
Fixed it! Should now be working everywhere 😅
Yeah, we can always go back and add classes to other blocks. If we were removing existing classes it would break back-compat, but this way it's fine. |
|
I like this a lot:
It feels like the right direction for me. Should we land this PR as-is? Then I can follow up separately on adding the new classes as the canonical styling classes, perhaps add some deprecation comments about the old classes. If yes, then it seems like the next thing is landing this one. @gwwar since you touched adjacent concepts for the Home Link, if you have bandwidth, can you take a look? Thank you. |
|
I'd like to add a common submenu class before merging, because that will help fix a bug I just noticed with the Page list submenus on a dark background:
It shouldn't take too long, working on it now. |
|
Ok, I added I'm thinking, once we've cleaned up the CSS, we needn't keep the old classnames for any of the Navigation-related blocks, because Navigation is still experimental. We'll have to keep them for the Page List block though, as it's already in Core and also it can be used standalone. We should still keep the top-level classname for each individual block, e.g. |
There was a problem hiding this comment.
This seems like a really good improvement.
As @jasmussen says, it passes responsibility to the Nav block for sorting out the layout of its expected child blocks.
Results
This tested well for me except I noticed that the Page List <ul> doesn't have the classes. I assume it would as you would want to space it away from the other "items". I assume this is done because in actual fact you want to space the "items" that are within the Page List block rather than the block itself?
✅ Frontend
✅ Editor
Concern
Nit: I was slightly concerned by the addition of is_navigation_child. Are we in danger of coupling Page List too tightly to Navigation? Would there be a way of allowing Nav block to adjust the markup of Page List rather than the other way around?
Yeah, the styling has to be added to the
Hmm, that's a very good question 😄 I don't think Page List is tightly coupled to Navigation in the sense that it can be used as a standalone block, and as standalone, there's no visible evidence of its relationship with Navigation. What could a mechanism to adjust children's markup in the Nav block look like? Currently the only block that needs these common classes and can also be used outside of Nav is Page List, but there will be more in the future, and they all need different classes added in different places, so potentially that would mean a lot of extra logic in the Nav block. What are the advantages of doing it? And what are the downsides of a block containing logic that relates to its parent? (I'm asking all these questions because I really don't know how to answer your original question 😅) I think problems with Navigation can be hard to think through because they're often unique to this block, so we can't look to other blocks for ideas. In many ways, it doesn't want to behave like a block at all: it has so many constraints about what can and can't go into it, as well as blocks that can only be used inside it. So it makes sense that a lot of the solutions we find will also be specific to this block, and not applicable elsewhere. |
The Page List is useful as a standalone block, I know @mkaz has pondered a "Show subpages only" toggle to it. It's also great in the Navigation block as it keeps things in sync. The server side nature of it, though, has meant it wasn't able to easily receive color properties in the recent separate overlay color PR. I don't imagine it would be necessary to write an additional Page List block to accommodate this, but if it were, it could be tied directly to it to the point that it wasn't allowed outside. Alternatively, could we tie the classname output of "is_navigation_child" to only be output when the Page List is a child of the navigation block? I can't speak to the feasibility or whether it's good practice, just rubber-ducking really, because as suggested above, I do sort of share the same concern.
I'm not certain I'm understanding you completely, but I read this as speaking to the structure differential between page list items and direct nav items, like so: At the moment, the CSS of the navigation block is designed so that visually the above looks as if it's just a flat list of 4 menu items, mostly ignoring the Page List container itself. It's possible that's not necessarily the best way, I'll try and revisit that when I get a chance to refactor the CSS pending these class changes. Tangential to that, I discovered that there's a frontend/backend markup differential as well, best described on the original PR: #32367 (comment) |
This already happens! The only coupling is in the actual logic, because Page List needs to know who its parent is, and make decisions based on that. But I think that's less messy than having Navigation take responsibility for all its children's output (unless there's a neat way to do this that I'm not seeing, which is why I was asking all the questions above 😅 ) |
I was just thinking Navigation could look at it's inner blocks and then add the classNames to them as required. I think Columns manages it's own inner blocks a fair amount: As The only other way I was thinking was via the classname filter. |
Hmm, Columns block is setting the attributes of its inner blocks, and then each Column still has to work out what to do with them. An easier way to do something like that here, given we're using the same classnames on all child blocks, would be to pass them down via block context. The advantage of that is we could declare all the classnames in a single place instead of hardcoding them in each child block, but it comes at the expense of added complexity in the child blocks, because we'll have to check that each classname exists in context before adding it to the markup. For some of the blocks (Nav link, Home link) this is overkill because they are always children of Nav, so hardcoding the classnames in is simpler. We could only do it for Page List, and hardcode the classes in the other blocks, but that sort of defeats the point. The other thing I'm thinking is that with The context thing might be useful if we ever have a block that can inherit classnames from more than one parent, but this is something we can always change in the future if needed, as it won't affect the output markup in any way. We wouldn't be able to use the classname filter here, because it only sets classes at the block wrapper level, and we need to set classes in several places inside the block. |
|
Sounds like my idea is just making things overly complex. Let's run with the prop on the Page List for now and we can always refactor later if necessary. Sorry for derailing things here but I think it's been a useful exploration of the alternatives 🙇♂️ |
|
As a followup I've started work in #34171 to leverage these classes. If you have bandwidth I'd love just a small sanity check on the work in progress to see if I haven't missed anything. So far it's working perfectly, though there are a few page list edgecases I'd like to tweak. An additional question: right now the Page List and Home Link blocks are changed to output the generic classname when used inside. That's fine — but would it be possible for the navigation block to handle that classname assignment? I think I've asked a mostly identical question on the Site Logo block PR 🙈 |
What we can do (and would work well with the Site Logo PR too) is pass the classname to the child blocks via Navigation block context. The advantage of that is the classname is declared by the parent. However, we'll still need logic in the child blocks to detect if the context exists and, if so, apply the classnames in the correct places. There's no way of having the Navigation block alter its children's markup, if that's what you mean. |
|
Thanks for clarifying. Sounds like that might be an improvement, but I'll defer to you. It's mostly a thought out of ensuring 3rd party menu item blocks can get the right positioning rules like everything else. But let me think a bit more about that. |
That's a very good point I hadn't considered 😄 Currently the Nav block only accepts a list of a few core blocks as children. I'm not sure it's possible to add to that list through a filter or something, but if it were, that would also raise the semantics issue: currently we wrap menu items in a list element, and for that purpose we decide what a menu item is. That list isn't filterable and probably shouldn't be as it's an implementation detail. Do we have any examples of 3rd party blocks specifically meant for Navigation? For mega menus, things might change as we'll be allowing all sorts of blocks - but I reckon that for anything that isn't a simple list of menu items (with possible sub-lists), we can build the layout with just CSS. The biggest problem we were having here, that the generic classnames solve, is that there are several blocks essentially adding the same type of content (menu items and submenus) to Navigation, and we need them all to look the same. With different types of content, this won't be so much of an issue. |
Dug up an older pull request that enables adding additional items to the menu, this one: #31584. It seems that particular approach might not be gaining traction, but the principle appears to have been enabling inserting something like a WooCommerce category link, which would then suggest woo categories in the dropdown. Though per your description above it seems like we'd be able to handle the classnames even in that use case. |
Oh, I see. That PR would only be adding new variations to the core Navigation Link block, not new blocks altogether. It would still be a core block, so it would have all the default classnames. |
Description
Fixes #33048. To make it easier to style children of the Navigation block consistently, we should have generic classnames that apply to all of them. This PR adds those classnames to Navigation Link and Home Link blocks, and conditionally to Page List when it is a child of Navigation.
To make the changes easier to review, this PR doesn't touch any CSS; we can consolidate that separately after the new classnames are sorted out.
Question: should we add the same classnames to any other blocks that can be used inside Navigation, such as Social Icons or Spacer?
How has this been tested?
Create a Navigation block containing the following blocks:
Check that
wp-block-navigation-itemis added to all the menu itemswp-block-navigation-item__contentis added to their content.Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).