Try using flex layout on the Navigation block #36169
Conversation
|
Size Change: -992 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
|
This is wild, I had not expected you to address that feedback in this PR! Thank you, the one-row of two toggle groups looks much better:
I suspect there might be tweaks to do as we get further on the component system and #27331, which primarily works with two columns. I suspect we might even want to take inspiration from the work here to affect the layout panel for all flex blocks, perhaps including the Row block. All that is unrelated and not blocking, and this feels like a good baseline to start with. Impressive work! 👍 👍 |
|
This is great. I think it would be good to seriously consider progressing with this for 5.9, and then seeing whether it's possible to address any remaining bugs during the beta phase. If it doesn't work out, the change can alway be reverted back.
This isn't something unique to nav block. Also an issue in the 'row' block variation, so I think you could consider it as something not introduced here. Would be good to solve for 5.9, but I feel it could be tackled as a separate effort. |
noisysocks
added
[Block] Navigation
tellthemachines
requested review from
ellatrix,
spacedmonkey and
TimothyBJacobs
as code owners
tellthemachines
force-pushed
the
try/navigation-flex-layout
branch
from
1d51ed3
to
c74ebab
Compare
|
@jasmussen this PR adds layout to the Navigation block. There are lots of CSS changes I'd appreciate your feedback on! In the meantime, I've fixed justification for the overlay menu.
Oh, I hadn't noticed it happening in Row. It makes sense to find a common approach for all these cases, then. Is there an issue for it already? |
I had a look and I don't think so. |
|
I created #36197 so we can look at the Spacer issue. |
|
I was clearly confused when I first tested this one, sorry. I read it as applying to the Buttons block and was excited about bringing the orientation controls to the Row block in the future. Now I took the Navigation block for a spin, thanks for your patience. Here's the new panel, working as intended: Lovely work, and an important refactor. The next challenge at hand is managing the prominence and hierarchy of the inspector. Valuable customization options that are likely to be the most interesting to someone customizing their navigation, are being pushed downwards still more: These are ultimately separate issues to solve, but nevertheless something to be mindful of returning to. For the Layout panel, we might be reverting the "Allow to wrap" control, as it did not bring as much value as we had hoped, that would help. For the naming panel, it seems like it could echo the Template Part block and put these controls in the "Advanced" panel:
The reason I bring it up is that additional visual customization options for the block will arrive in the future, so it's a challenge we will need to consider in the near future. I found a few issues compared to trunk. First off, the overlay when opened inside the editor is offset:
And the justifications inside the overlay menu do not work on the frontend. Some of the behaviors of that overlay menu are a bit gnarly and could use a refactor in the future, but as it exists, the overlay is designed to be a simple toggle with no further content customizations inside, meaning if you justify the menu right, the content of the menu is also justified right. See #36105 for some more gnarly details. I'll dive into the CSS in a moment, and I'll see if I can push some fixes as well. One other thing I noted, is an issue also present in trunk, so unrelated, but storing as I noticed it here: the transforms section of the URL dialog is empty when used inside submenus. We can probably just hide that section entirely for submenus: |
| @@ -441,28 +442,28 @@ $color-control-label-height: 20px; | |||
| } | |||
| } | |||
|
|
|||
| .has-fixed-toolbar .wp-block-navigation__responsive-container.is-menu-open { | |||
| .has-fixed-toolbar .wp-block-navigation.is-responsive-menu-open { | |||
There was a problem hiding this comment.
I like the new simpler classnames. Hopefully it's just a missing update that's causing the justifications to not work on the frontend, pretty sure there are explicit justification rules that target this one.
There was a problem hiding this comment.
Did you test the latest changes to this branch? Overlay menu justification wasn't working in the first iteration, but I've fixed it since. If you're testing locally, you might have to delete your local PR branch and check it out again, because I rebased it yesterday.
| @@ -327,107 +314,29 @@ | |||
| .is-responsive { | |||
| display: none; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
So much beautiful red here 👌
| // Justification. | ||
| // These target the named container class to work even with the additional mobile menu containers. | ||
| .items-justified-center { | ||
| .wp-block-page-list, |
There was a problem hiding this comment.
These rules might be ones we need to restore, at least the responsive container parts as they affect the contents of the overlay.
Essentially, if you justify your navigation block to be centered, so should the content of the overlay be centered. The tricky piece comes with the dual variations, horizontal and vertical for the block itself, regardless of that orientation, the content of the overlay should always be vertically aligned to the top, so we can't just inherit the rules, and we have to have separate rules for the vertical and horizontal orientations.
There was a problem hiding this comment.
Justification inside the overlay should be working now. In any case, these styles no longer apply when using layout because items-justified-center etc. classnames aren't added to the markup.
The tricky piece comes with the dual variations
😅 yup, it was tricky all right. In order to make it work, I added a custom property --justification-setting to the layout-generated styles that get added to the block container. That property is then used to set justification inside the overlay regardless of whether the block is oriented vertically or horizontally. In my local it works well and content is always vertically aligned to the top, but as I mentioned above you may need to pull down this branch again to test the latest version.
| align-items: flex-start; | ||
| justify-content: flex-start; | ||
| // Give it a z-index just higher than the adminbar. | ||
| z-index: 100000; |
There was a problem hiding this comment.
I'm curious why we needed to change this z index.
There was a problem hiding this comment.
|
|
||
| // Always vertically align to the top. |
There was a problem hiding this comment.
This is what I referred to previously, the content of the overlay modal should always be vertically aligned to the top, regardless of whatever parent orientation we have.
| // Always vertically align to the top. | ||
| justify-content: flex-start; | ||
| .wp-block-navigation__responsive-container-content { | ||
| display: contents; |
There was a problem hiding this comment.
I'm not sure what this does. I understand that it makes the wrapping element invisible, but is that to simplify the inheritance rules? Keeping in mind that align-items and justify-content sort of flip when the orientation of the block itself does, unfortunately we can't just inherit those.
There was a problem hiding this comment.
Yeah, so display: contents works when the nav is not in responsive mode. When in responsive mode, it's overridden by the custom styles that display the overlay contents as a column, and alignment/justification are handled by the --justification-setting custom property.
| justify-content: flex-start; | ||
| // Give it a z-index just higher than the adminbar. | ||
| z-index: 100000; | ||
| padding: $grid-unit-40*2 $grid-unit-30 $grid-unit-30; |
There was a problem hiding this comment.
Normally I'd appreciate the move to variables. But since this is the style.scss file meant for the frontend as well, I don't think it's appropriate. The variables are mainly meant for block UI.
|
It looks like some of the inherited layout classes ( The max width and margins can be handled with these !importants which feel appropriate enough given they are meant to override other !importants: |
|
The explicit submenu/overlay color options appear to have broken. They don't appear to be set at all: CC: @georgeh as I recall you working on this feature. |
|
The alignments inheritance is really difficult to get right. Your effort here is quite impressive, but I think we might need to rewind some of the CSS refactor, as the inheritance doesn't appear to be working as intended: Would it be possible to do this refactor in smaller steps? |
@jasmussen I get that this doesn't look great, but it saddens me that you've decided to ignore the feedback of your fellow collaborators. I really tried to find a compromise on this issue - #35981 (comment). I feel like my feedback was ignored, and now it looks like you've decided to get around this by asking another contributor to implement the changes you want. As mentioned on that issue, the navigation block differs from template parts. Template parts are always given a meaningful name on creation. Since #36024, the navigation block can have 'Untitled' menus where the user isn't given an opportunity to name them, so naming isn't only an advanced action. I am very open to finding ways to improve the appearance. Making the panel less prominent. But I'd really prefer not to relegate it completely to the Advanced section. Can we come to a compromise here that makes us both happy? |
|
I want to be clear that I'm not asking anyone to implement changes here they do not support, it's separate from this PR. I bring it up in context of the layout panel introduced compounding the same issue: pushing downwards valuable appearance options for the menu. I genuinely think it's problematic to give prominence to a data store in a way that no other block does: if it truly is important to save, name and manage the states of blocks, then inversely we should bump the prominence of the same interface in the Template Part block. It's fine that things are in flux and being explored. I've intentionally not blocked any PR from happening, so that PRs can land and be acted upon. |
I think this is a hazy area, it's probably down to the release lead, but as a small change it might be ok to include after feature freeze. I'd prefer to be on the safe side and address it before that point so that it doesn't get stuck in a position that you really disagree with. Moving it down sounds good, I'll work on that tomorrow, and if you have any other ideas for temporary improvements to the panel itself I can look at those too. |
|
I'm catching up a bit with the recent activity here.
I think this is the problem we need to address, everything else just spirals out from it.
It's crucial to me that saving something separately doesn't incur in extra over head for users because we'd want to leverage these mechanisms in more places — for example, the "overlay" we see on collapsed blocks is something we could conceptualize as a template part, but we wouldn't want users to name them or be exposed to that abstraction layer. |
I can reproduce this on trunk too, so I think it's an underlying issue. The overlay is just inheriting the container backround color, so whatever background is set on the nav will appear on the overlay too.
I'm having trouble understanding that gif; there's a lot going on there 😅 (Apart from the overlay not stretching full width; I'm looking into that now). Could you give me some steps to reproduce/expected behaviour? The content alignment seems to be working well for me locally in both the editor and the front end.
I'm afraid that won't work. Layout breaks a lot of existing functionality for the nav block, so we need to make sure it's all done in one go to avoid regressions 😬 |
Ok I fixed this by reverting some of the styling changes. Initially I had attached the overlay styles to the block container, but this is no longer necessary because we don't need all the wrappers to I think that's all the issues addressed! This should be ready for another round of review. It would be great to get it in in the next few hours, otherwise it won't make 5.9. |
|
This has made great progress! The overlay alignments do appear to work as intended. There are a few things, and I'm currently looking at whether I can push fixes for it. The submenu colors do work in trunk, just not with the new Page List block:
Now they do appear to take 🤔 at least partially:
I'm separately looking at some refinements to the direction submenus open in (#36215), but here that appears to not work fully as intended. When a menu is justified right, submenus should open leftwards, rather than rightwards: It might just be one of the rules that isn't reaching its goal anymore. I'll see if I can do anything. Edit: the spacing/gap between items also appears to be off. |
|
Cool beans, the block spacing issue was due to TT1 blocks not opting into that option, so actually the previous note on spacing being up was a bug that this PR fixes:
|
|
It seems like the menu-opening-directions is a cause of the justification classes now missing. These are the rules that govern the direction the submenu opens: Because they leverage the CSS classes applied for justifications, and those classes are missing, the rules now don't take. Can we add those classes back? |
|
I don't know why the submenu colors are applied to 2nd level nested items, but not the 1st level nested items, you can see the background color applied in this markup on the right: So as a quick summary for merging and fixing. The good stuff:
The missing pieces:
It might be okay to land this PR as is, with the missing pieces receiving quick followups. But if there are some quick fixes we can make, it'd be nice. Note that the submenu background colors appear to be broken for the Page List also in trunk, so that's separate. |
|
Thanks for the feedback and testing @jasmussen ! Let's look at what still needs to be addressed:
This is reproducible on trunk, and as far as I can tell it happens because the inline styles that provide the colors aren't being applied to the Submenu block at all. So in a submenu nested 2 deep, the first child item will be a submenu, so no color, and the second child item will presumably be a nav link block, so it gets the color. It shouldn't be too hard to fix but better do it separately.
This is a thornier issue because with layout those classes are no longer applied. The whole idea of layout is to make it unnecessary to add specific classes (and block-specific styles for those classes) to blocks. So our options here are:
I also noticed the open on click functionality for submenus seems to be broken on trunk, so that's one more thing to follow up on separately. |
There was a problem hiding this comment.
I'm ok with the idea of fixing the remaining issues during the beta period. It seems like this is 90% there, and it'd be a shame for it not to make it.
I'll give the PR a rebase.
@tellthemachines How confident are you about fixing those problems?
| }, | ||
| isEligible: ( { itemsJustification, orientation } ) => | ||
| !! itemsJustification || !! orientation, | ||
| migrate: migrateWithLayout, |
There was a problem hiding this comment.
I'm cautious about this because of the discussion here - #35663.
I'm yet to write a migration and don't really fully understand them. A question I'd have is why v4 needs migrateFontFamily, but v5 doesn't? The definition of the two deprecations for font family looks identical.
There was a problem hiding this comment.
I more or less followed the logic used in #35819, as I realised in that PR that if the legacy justification mechanism isn't migrated, it's impossible to justify an old block using the new layout controls, because they're always overridden by the legacy styles. This may not apply exactly to nav as it's still experimental, but I added it in to be on the safe side.
I think this is something we can safely look at later though.
|
If we have confidence in the ability to fix the remaining issues as bugs, that's good.
I suspect #36241 may be the better long term solution. That would make submenu direction independant of justifications. However in the near term, TwentyTwentyTwo is relying heavily on a right-justified navigation menu, as well as submenus, so I suspect we'll want to keep those classes at least in the near term. It's a behavior that's been refined for a while and is generally working well, so I'd be hesitant to remove it entirely. Even now, there's a great deal of nuance in how submenus open, and a lot of edgecases which are easy to forget, so I suspect a lot of that will have to be polished up if we do land this, and I'd appreciate help in doing so. #36215 for example is likely going to need to be rewritten for the changed structure. |
talldan
force-pushed
the
try/navigation-flex-layout
branch
from
1fdf183
to
e387c62
Compare
| @@ -535,8 +435,7 @@ | |||
| @include break-small() { | |||
| &:not(.hidden-by-default) { | |||
| &:not(.is-menu-open) { | |||
| display: flex; | |||
| flex-direction: row; | |||
| display: contents; | |||
There was a problem hiding this comment.
Is the Safari/a11y bug relevant here (and other places contents is used)?
There was a problem hiding this comment.
Oooh, good point! I'll look into it.
There was a problem hiding this comment.
Can confirm display: contents wipes out list semantics. Looking at a fix now. Thanks for bringing it up!
There was a problem hiding this comment.
And here's a fix : #36292
There was a problem hiding this comment.
No problem, kudos for digging into that and figuring out an alternative 👍🏻
Description
Closes #34872
This is quite a large changeset, mainly because of getting layout to work with the responsive wrapper. In order for all the navigation inner blocks to inherit the flex layout from the block wrapper, I had to set all the intermediate elements to
display: contents, and in order for that to work with the responsive menu overlay, I had to move most of the overlay styles to the block wrapper.One thing I haven't quite solved is how to force all the inner blocks to display as a column inside the overlay menu, because
display: contentsmeans the inner blocks just inherit whatever is set by the layout controls.The other thing I haven't solved is how to get a nested Spacer block to inherit the orientation from the Navigation block, given that
orientationis no longer a block attribute.There may be a few other rough edges here and there 😄
This is a bit of an experiment to see if we can/want to do this, so any and all feedback and ideas welcome!
How has this been tested?
Add a Navigation block; change its justification and orientation properties and check that all works as expected.
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).