Refactor navigation block to use generic classnames. #34171
Conversation
|
Size Change: -1.01 kB (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
| @@ -1,8 +1,12 @@ | |||
| // Navigation block and menu item styles. | |||
| // This also styles navigation links inside the Page List block, | |||
| // as that block is meant to behave as menu items when leveraged. | |||
There was a problem hiding this comment.
Shouldn't this line also be removed?
| .submenu-container, // This target items created by the Page List block. | ||
| .wp-block-navigation__container .wp-block-navigation-link__container { | ||
| .wp-block-navigation__container .wp-block-navigation__submenu-container { |
There was a problem hiding this comment.
We should be able to remove submenu-container here, and use just .wp-block-navigation__submenu-container to target all submenus. I'm not sure why the .wp-block-navigation__container was added in the first place; it's not needed to target only the submenus, because the submenu classname does that already, so it may have been added to increase specificity. But if we do need additional specificity, we can just duplicate .wp-block-navigation__submenu-container.
|
Thanks so much for the review 👌 — I will address your feedback and return! |
jasmussen
force-pushed
the
update/navigation-generic-classes
branch
from
afee40f
to
01189e1
Compare
jasmussen
requested review from
adamziel,
ajitbohra,
draganescu,
kevin940726 and
talldan
as code owners
jasmussen
added
[Block] Navigation
|
Thanks again for the review, I've addressed your points by implementing your suggestions one by one and it appears to be working really well all over. I'd still appreciate a good thorough review on this one, here's some example content: One thing I noticed, the sub-submenu indicator in nav items look like this:
The chevron is inside the a element:
The Page List looks like this:
The chevron is outside of the a element:
I should be able to tweak that by editing the Page List block itself — any concerns with that? As an additional question, now that we've refactored the nav block classnames, can we/should we retire or remove some of the existing CSS classes applied? Page list is probably a challenge since it's shipping, but what can or should we do for the nav block itself? Note to self, as a followup I'd like to look at an issue where the submenus take up horizontal space even when they are invisible, in some cases causing horizontal scrollbars. |
There was a problem hiding this comment.
This is looking good!
Unfortunately the example markup doesn't work on my local because all my post and page URLs are different 😅 but I tested with both horizontal and vertical variations of Navigation, with multiple nested submenus on both Navigation items and Page list, so I think all the possibilities have been covered.
(I did come across a couple of styling issues, but they're already on trunk so we can deal with them separately)
I should be able to tweak that by editing the Page List block itself — any concerns with that?
It might be good to address that separately too, because it might conflict with the work we're doing in #33775 to enable open on click instead of focus. That PR doesn't touch Page List, but we will want Page List submenus to also open on click so they match the rest of the Nav, right?
As an additional question, now that we've refactored the nav block classnames, can we/should we retire or remove some of the existing CSS classes applied? Page list is probably a challenge since it's shipping, but what can or should we do for the nav block itself?
Yes, we definitely should! And yes, we might want to leave Page List classes alone because the block is already in Core, but for the Navigation block and its specific children, we can remove the old classes. Again, best do a follow-up PR so we can merge this one already 😄
jasmussen
force-pushed
the
update/navigation-generic-classes
branch
from
15fb931
to
8994a78
Compare
|
Rebased, it seems like this one could be good to land soon!
Ideally the Page List would behave the same as any other menu items, yes. Speaking of, I've explored some designs that leverage such a dropdown menu block but with a parent link inside, that considers the Page List as part of the equation. |
|
The justify items do not seem to be working properly in the editor, but do appear to be in published view. |
Navigation links look like this on TT1; on your branch, and on
I'm wondering if the arrow being stuck to the text is what's expected here, or if the separation would be better. If the former is true, then I think modifying the Page List block would be OK! Otherwise, we might win something by changing the navigation link markup instead, and helping TT1 to not look — in my humble opinion — this wonky. |
Yeah, this is one of the issues I mentioned above. It's happening across the board, not only on TT1. It should also be mentioned that the formatting is different depending on whether the submenu is selected, or its parent. |
Oh, well spotted, @mkaz ! Justification isn't working at all for vertical nav on trunk either, so not a problem specific to this PR. For the horizontal nav though, it's the margins on the children that are being overridden by general block styles: |
Great call and thanks for the advice. I'm hesitant to do it in this PR, though, and in fact I'm hesitant to even do it at all. The rules are output by classic.scss file, i.e. the issue should not be present on block themes. In #32659 I tried my best to reduce the specificity of those classic styles so that we wouldn't have to increase specificity otherwise. The problem with increasing specificity is that we need low specificity for global styles/theme.json to be able to set the margins, as #31878 sought to achieve. There's likely still a solution for classic themes, but I'd like to explore it separately. In addition to this, TT1 has obsolete rules for styling the navigation block, which is causing those extra paddings. I've tracked that here: https://core.trac.wordpress.org/ticket/53220 So unless there are any objections, I think I'll land this in an hour or so, just to unblock other related PRs that rely on this. Let me know otherwise! |
|
Alright, landing this one and as always, happy to do followups. |
|
Should this one get a dev note? In case yes, here's a suggested note:
|
|
It won't need an actual dev note on make/core because this block isn't in core yet, but it might be good to mention it in the plugin release notes. |
|
@jasmussen @tellthemachines Please can you make a point of testing the navigation editor with any block changes. Things are looking a bit of a mess with all the changes to the block over the last couple of months. |
| @@ -131,15 +131,15 @@ | |||
|
|
|||
| // Override for deeply nested submenus. | |||
| .has-child .wp-block-navigation__container .wp-block-navigation__container, | |||
| .has-child .wp-block-navigation__container .wp-block-navigation-link__container { | |||
| .has-child .wp-block-navigation__container .wp-block-navigation__submenu-container { | |||
| left: auto; | |||
| } | |||
|
|
|||
| // When editing a link with children, highlight the parent | |||
| // and adjust the spacing and submenu icon. | |||
| .wp-block-navigation-link.has-child.is-editing { | |||
There was a problem hiding this comment.
Is .wp-block-navigation-link still a valid classname? I think this might be where the problem is. Should it be wp-block-navigation-item?
There was a problem hiding this comment.
It should be, yes. Fixed in #34344.
| @@ -43,8 +43,8 @@ $z-layers: ( | |||
| ".wp-block-template-part__placeholder-preview-filter-input": 1, | |||
|
|
|||
| // Navigation menu dropdown. | |||
| ".has-child .wp-block-navigation-link__container": 28, | |||
| ".has-child:hover .wp-block-navigation-link__container": 29, | |||
| ".has-child .wp-block-navigation__submenu-container": 28, | |||
There was a problem hiding this comment.
It'd be great to have some docs in the navigation block's readme about how these classnames work.
There was a problem hiding this comment.
I took a stab at that here: https://github.com/WordPress/gutenberg/pull/34344/files#diff-b43a36c885bf6479506b3541da11760e584757fd78809954b3ff785f54a2e11dR47 — let me know if that looks right to you.
|
Ok, I think I've managed to fix up many of the broken styles in the nav editor as part of #34281 |
|
Thanks for looking, Dan, sorry about the nav screen issues. I will create PRs for your points and let me know how I can help with 34281. |
|
@jasmussen No worries, no need to make PRs as the issues should be fixed in #34281. A review would be appreciated 😄 I think we just need to make sure the nav editor undergoes a comparison with |
|
I know you already have some of the navigation screen issues fixed, thanks for that. Nevertheless I created #34344 to fix a few additional issues, so some overlap did happen. However your #34281 still includes some fixes to the chevron open/close behavior that 34344 does not, so we'll eventually need both. |
Description
This is a work in progress and a followup to #33918. The goal of this PR is to simplify the navigation block CSS to target menu items in a generic way, so as to avoid drift when new menu items are added.
Checklist:
*.native.jsfiles for terms that need renaming or removal).