Refactor and simplify navigation block CSS. - #29465
Conversation
| @@ -1,17 +1,23 @@ | |||
| // Normalize Link and edit containers, to look mostly the same. | |||
| .wp-block-navigation-link__field .components-text-control__input.components-text-control__input, | |||
| .wp-block-navigation-link__field .components-text-control__input.components-text-control__input, // @todo: this selector doesn't appear to target anything. | |||
There was a problem hiding this comment.
I could not find any instance of the wp-block-navigation-link__field except in this style.
There was a problem hiding this comment.
Same here. Should we just remove it now?
There was a problem hiding this comment.
+1 for removing unused rules. If we're unsure we can maybe follow up in a second second PR
| border-radius: 0; | ||
|
|
||
| // Make it the same height as the appender to prevent a jump. Maybe revisit this. | ||
| line-height: $button-size; |
There was a problem hiding this comment.
I could potentially remove the "Maybe revisit this" comment, as this is now revisited. Submenus should be allowed to inherit line height rules from the theme, in editor and frontend, and it does not appear to cause issues to remove it.
|
|
||
| // Separator | ||
| // Separator. @todo: this appears unused. | ||
| .wp-block-navigation-link__separator { |
There was a problem hiding this comment.
I could not find anything that used this class.
There was a problem hiding this comment.
Like the one above, I suggest we just remove this. We can always add it back in if someone notices something we missed.
| // several times, so care needs to be taken. | ||
| background-color: $white; | ||
| color: $gray-900; | ||
| background-color: #fff; |
There was a problem hiding this comment.
Since this is the style.scss file, it should use plain hex colors, not the editor UI variables.
There was a problem hiding this comment.
This make sense for now. For block-based themes though, I wonder if we can automatically pull this in from the default text/background colors as defined in theme.json. 🤔
There was a problem hiding this comment.
We definitely should. In the mean time, this was a teensy "while I'm here" change.
| } | ||
| // These need extra specificity. | ||
| .editor-styles-wrapper .wp-block-navigation { | ||
| ul { |
There was a problem hiding this comment.
I removed the "ol" rule here, as it does not appear we use the ordered list for structure, anywhere in the code.
| } | ||
| } | ||
|
|
||
| .wp-block-navigation__inserter-content { |
| } | ||
| } | ||
|
|
||
| // Show submenus on hover. |
There was a problem hiding this comment.
Has the context for the decision to show these on hover changed? We could potentially remove the sensitive hover/focus-within rules entirely, and rely on &.is-selected or &.has-child-selected to keep it open. This would open the menu not just on hover, but on select too, and keep it open until deselected. This seems like it could simplify both the CSS and the user experience, and avoid the IE polyfill that is noted as necessary.
There was a problem hiding this comment.
I sm a fan of removing this on hover behavior and sticking to selection when editing. The only thing in my head to consider is that if the menu is indeed opening on hover when rendered, the user should be awayre that the edit behavior is different than the rendered behavior.
There was a problem hiding this comment.
Just in case there's any doubt, this PR doesn't change the behavior yet, just added a comment to describe the current behavior.
But yes, I think we can be opinionated about the edit behavior and keep it open on select. But we could also show it on hover, and keep that behavior. And we'd still be able to remove the need for the suggested polyfill.
Once this PR lands, I'll try this separately, we can see how it feels 👍
| */ | ||
|
|
||
| // Ensure that an empty block has space around the appender. | ||
| // @todo: revisit this to allow thinner menus. |
There was a problem hiding this comment.
The min-height should ideally only be applied to the setup state showing the "skeleton indicators", otherwise we make it harder for themes to create small menus, like this one:
But I'd like to do this separately.
| ul li { | ||
| list-style: none; | ||
|
|
||
| // Overrides generic ".entry-content li" styles on the front end. |
There was a problem hiding this comment.
These styles were previously in the theme.scss stylesheet, but are structural so I moved them here.
I also simplified them and tweaked their specificity, which is still pretty low. I did verify they overrode generic .entry-content li margin rules a theme might add.
|
Size Change: -1.14 kB (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
cf6ac5e to
e8d1ab3
Compare
|
Rebased and squashed to make it easier to rebase in the future. |
|
I'd be very grateful for reviews of this one. |
kjellr
left a comment
kjellr
left a comment
There was a problem hiding this comment.
I did some testing in TT1 Blocks, as well as a few non-block-based themes, and didn't notice any obvious regressions. I left some minor notes, but I think this seems fine to land either way.
| @@ -1,17 +1,23 @@ | |||
| // Normalize Link and edit containers, to look mostly the same. | |||
| .wp-block-navigation-link__field .components-text-control__input.components-text-control__input, | |||
| .wp-block-navigation-link__field .components-text-control__input.components-text-control__input, // @todo: this selector doesn't appear to target anything. | |||
There was a problem hiding this comment.
Same here. Should we just remove it now?
| border-radius: 0; | ||
|
|
||
| // Make it the same height as the appender to prevent a jump. Maybe revisit this. | ||
| line-height: $button-size; |
|
|
||
| // Separator | ||
| // Separator. @todo: this appears unused. | ||
| .wp-block-navigation-link__separator { |
There was a problem hiding this comment.
Like the one above, I suggest we just remove this. We can always add it back in if someone notices something we missed.
| } | ||
|
|
||
| // Popover styles | ||
| // Popover styles. @todo: these all appear unused. |
There was a problem hiding this comment.
If this is also unused (which it seems to be), it seems like a possible candidate for removal too.
| // several times, so care needs to be taken. | ||
| background-color: $white; | ||
| color: $gray-900; | ||
| background-color: #fff; |
There was a problem hiding this comment.
This make sense for now. For block-based themes though, I wonder if we can automatically pull this in from the default text/background colors as defined in theme.json. 🤔
|
Thanks so much for the review, Kjell! I'll address your feedback, then land it sometime early tomorrow, so others can still chime in if they like. |
| ".wp-block-template-part__placeholder-preview-filter-input": 1, | ||
|
|
||
| // Navigation menu dropdown. | ||
| ".has-child .wp-block-navigation-link__container": 28, |
There was a problem hiding this comment.
What value is the sibling inserter?
There was a problem hiding this comment.
Things like the block toolbar and the sibling inserter have been moved to "popovers", that is, children of the block-editor-block-list__insertion-point-popover element that sits outside the editing canvas and is then overlaid. That element has z-index 28 also:
gwwar
left a comment
gwwar
left a comment
There was a problem hiding this comment.
Thanks @jasmussen for simplifying styles. ✨
I did a bit of smoke testing, but wasn't able to spot differences. Another +1 review wouldn't hurt.
It may help other reviewers to rebase this PR to pickup the typing fix for navigation links. (Previously editing would break the block).
e8d1ab3 to
f8c841a
Compare
|
Great point Kerry, freshly rebased! Thanks so much for the reviews. I'll keep this one open and address the points brought up tomorrow, and re-request a review if the changes are substantial, and in the mean time, I'd welcome all the sanity checks you can afford. |
shaunandrews
left a comment
shaunandrews
left a comment
There was a problem hiding this comment.
All seems well. Skimming the code and all seems to make sense. Can't wait for further PR's that removes some of this, and reconsiders the decisions to show submenus on hover.
The custom select control does not appear used at all in the placeholder anymore. See also #23297.
3af0f03 to
1ed95ac
Compare
|
I've addressed the feedback, rebased, and the changes were small. It still tests well for me, and as I've noticed that this PR also fixes a small issue that was introduced by #29590, I'm going to land it when the checks pass. Thanks, all, for the reviews. I will of course address any followups. |
|
@jasmussen Has this been tested in the Navigation Editor at all? It looks to me like there already a few styling regressions there in |
|
I did test the earlier version, yes, but I'll give it another spin now after the rebases and tweaks. Thank you. |
|
I'm seeing the 'Add a link' button looks mis-sized and dropdown menus are back instead of accordions: But I don't think they're caused by this PR, so you may be fine to carry on. I can make an issue. |
|
This is what I see in trunk:
This is what I see in this branch:
Lots of little things to tweak, I'll see if I can help with that. But from what I can see, the experience is mostly the same between trunk and this branch. I'm not really seeing what you were seeing, not quite sure how to reproduce. |
|
I just tested one last time with the menu item placeholders, that seems to have been what you were referring to. I'm seeing this, and it looks reasonably correct in both (with the exception of the nav screen needing menu item placeholder polish). Trunk:
This branch:
|
|
I'm going to land this one and, again, follow up on any issues that come from it. |
|
@jasmussen The different styles you're seeing are because of this bug - #28874 If you try a different theme (e.g. normal TT1) you should see the same as me. |
6 participants
Description
The purpose of this PR is to refactor, cleanup, simplify and reduce the CSS for the navigation block. Ultimately this should benefit further CSS tweaks and pattern creation.
How has this been tested?
So as to have as basic an experience as possible, and to not be too influenced by theme styles initially, I tested using "Empty Theme", which you can get here: https://github.com/WordPress/theme-experiments
But it should also be tested at least with TT1 blocks, and other themes that style the navigation block.
Please test page list, manual page items, the setup state for both the navigation block and the menu item, test submenus. Please test editor and frontend.
You should not see any visual difference between this branch and trunk.
Screenshots
Editor, empty theme:
Frontend, empty theme:
Editor, TT1 Blocks:
Frontend, TT1 Blocks:
It also fixes a rare issue where you manage to activate the sibling inserter, with a submenu open. Before:
After:
Types of changes
A number of changes:
Checklist: