Allow Site Title and Logo inside Navigation block. #33316
Conversation
tellthemachines
added
[Block] Site Logo
|
Size Change: +6.79 kB (+1%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
|
This is really, really nice:
The above is a really basic navigation pattern that I think will be popular — Site Title would work well there also. I used |
|
I like this PR a lot, and it's very simple and neat. Revisiting it now, the primary thing stands out to me is the lack of margin:
The navigation block has CSS baked in that applies default margins to menu items inside. This: While we could duplicate that CSS or expand it to also target this block, the problem is that there are many compounding edgecases for those margins making it non trivial to duplicate, and at very high risk of drifting:
The CSS is already fairly gnarly, and this exact problem already affects the Home link item:
The current best suggestion for how to address this is #33048, which is to add more generic "menu item" CSS classes that can be targeted by the margin and positioning rules, so that we don't have to duplicate and expand the complexity for every new menu item added. I wouldn't necessarily object to this PR landing as-is, without addressing #33048 first. But it does seem slighly more urgently in need of being addressed. I'd love to pair with someone on fixing that, if anyone has bandwidth. |
As commented here, I'm happy to work on it as the problem does need to be addressed soon. The changes themselves should be straightforward, once we agree on the classnames. |
tellthemachines
force-pushed
the
add/site-title-in-nav
branch
from
11a195b
to
8b615bb
Compare
|
@jasmussen with #33918 merged, what's the best way forward here? Should Site Title and Logo conditionally display the If so, I'm thinking we should have the CSS cleaned up and using the new classnames before we do that, so it's easier to test how it works. |
In my dream scenario, the navigation block itself would apply classnames to its children based on their location inside. Would that be possible at all?
I'm spread across a few separate projects at the moment, but I will get to the CSS cleanup as soon as possible! Very excited about it, thanks again. |
It's possible to set classnames conditional on position via block attributes, but I need to look into what happens when the blocks are moved around inside navigation. For this to work, the parent block will need to be aware of changes in order of its inner blocks. What classnames would you like to apply to which locations? |
It's mostly a philosophy that the layout of child elements should be handled by the container if possible. In this case, due to the fact that we allow a variety of elements inside the navigation block (and that it can vary from editor to frontend), ideally we could just have a single generic CSS class that indicates: this is now a menu item. So it could be the same CSS class as you applied in that separate classname PR. What do you think? |
tellthemachines
force-pushed
the
add/site-title-in-nav
branch
from
8b615bb
to
c78512f
Compare
How is this being handled in other cases? I know there was some work done recently on the Group block; have there been any thoughts around adding classnames to child blocks? If so it would be best to try for a solution that works everywhere. Either way, I'm wondering if the context solution @getdave and I were discussing on the generic classnames PR might be a good fit. If it turns out that Site Title and Logo might need different custom classnames when they are children of some other block, passing a classname though context is a good solution. Even if they only need a classname when they're inside Navigation, we'll have to leverage some form of context to know when the blocks are inside Navigation. (None of that should change the work done in the previous PR though: Page List needs classnames added in several places in its markup, which would be unwieldy to pass through context.) |
No, I think adding child classes is a bit of an edgecase for the navigation block due to its complex layout and structure. I was thinking most in terms of layout and such, where the |
|
@jasmussen I've updated this PR to add the I guess this is now ready for a final review! |
|
Wonderful. I landed the CSS refactor (thanks again for the reviews), so a rebase on this one might unlock things. |
Description
Partially addresses #33278. Allows Site Title and Site Logo blocks to be used inside a Navigation block.
How has this been tested?
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).