Nav Block: Show justification controls for vertical variant #30351
Conversation
|
Size Change: +1.61 kB (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
|
Thank you Marcus, it's already been a long day for me so I'm going to take a look at this one tomorrow! |
jasmussen
force-pushed
the
add/29459-nav-vert-align
branch
from
80223ed
to
a9e44c9
Compare
|
Fixed the alignments. Let's get a sanity check and merge.
|
|
@jasmussen Ah, right Testing it out and it doesn't seem to be working properly for horizontal, the vertical list looks and works fine but horizontal changes are not being seen with the block selected, but only show when not selected. See GIF below,
|
|
Try deselecting the horizontal version once you apply an alignment. If that works, then it's a known issue partially mitigated by #29771 Edit: ah I see you did try deselecting. Yeah that should be present also in trunk. |
|
@jasmussen Confirmed, same behavior exists on trunk! No regression on this PR ✔️ |
|
I did more testing on other themes and in different regiions, both the Navigation Editor, and Site Editor Screens.
|
|
Awesome testing. Actually the fact that it affects the navigation screen — it probably shouldn't, which is to say @talldan might want to know. |
mkaz
requested review from
adamziel,
draganescu,
kevin940726,
noisysocks and
tellthemachines
as code owners
|
@jasmussen Good catch on the navigation editor screen! I tested it there and confirmed it worked, but didn't think if it makes sense there or not. Considering that screen is to create and edit menus for classic themes, the layout format won't work and should not be there. In 9ebbbab, I added a new attribute that can be used to disable the controls, and updated the Nav Editor to pass in this attribute so the justify controls will not show. Tested and confirmed all are showing everywhere else, but not there. 👍 @talldan Can you confirm this makes sense, and works as you would expect? 🙏 |
|
@mkaz Thanks for the ping. @jasmussen is right in that it shouldn't be on the navigation screen. This navigation screen previously disabled this using a prop that's passed into the block's edit function, but it looks like this PR removes that prop: Instead of removing it, you should be able to default it to I think it'd be good to avoid using an attribute for this purpose, as attributes shouldn't really be for editor side settings, more for the frontend representation of a block. Hope that helps. |
|
BTW - there is an issue to discuss better ways of enabling/disabling features in the nav block (#30007). I think content justification is one of the easier ones, in that it could be a candidate for becoming a block 'supports' feature. I know there's something very similar on Buttons block as well. One thing to check is that the changes here don't diverge too much from the Buttons implementation. |
|
@talldan After looking into it and rediscovering why we opted for this implementation in the first place. The way the justification works is it applies to the inner elements and can't be applied to the wrapping element the same for all uses. The supports mechanism doesn't allow for the level of customization needed. An example can be seen for Social Links implementation here: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/social-links/edit.js#L119 the Social Links uses its own function and specific CSS to apply properly to the correct inner elements. |
|
Ok. My comment here still applies - #30351 (comment) |
- Don't limit controls to show only on horizontal - This enables controls to show for both horiz and vert - The vertical CSS specifies a block so will require tweaking - Filter out space-between from vertical controls
jasmussen
force-pushed
the
add/29459-nav-vert-align
branch
from
32ee94d
to
53b3d69
Compare
|
Alright, to follow up on this one, I rebased to fix the conflict, and I made an additional fix. I realized text in menu items should be right-aligned. Before:
After:
Note that I tested the navigation screen again, and I couldn't see the justification controls there:
So that issue appears fixed. Or was there anything else we needed to do? |
mkaz
force-pushed
the
add/29459-nav-vert-align
branch
from
49fb988
to
0454ead
Compare
- Adds an additional attribute to showJustifyControls to navigation block so the Nav Editor screen can hide the controls, since that screen is intended to create/edit nav menus and not format/style. - Use 'showJustifyControls' since the positive is easier to reason and there is already a 'showSubMenuIcon' attribute that also defaults to true
mkaz
force-pushed
the
add/29459-nav-vert-align
branch
from
0454ead
to
9ed9056
Compare
|
@talldan Ok, updated with I think what you were saying. I was confusing props and attributes before. I went back to previous prop that was in place and switching that to default true, basically does everything we want, plus the additional updates in this PR for style and hiding space-between. If you can take a look now, I think its right and ready, and sorry for any confusion. 👍 |
Let me investigate that before I merge. Thank you for the review! |
|
In my theme I'm seeing the same before:
and after:
If you have a screenshot or steps to reproduce, I'm happy to take a look. |
|
@jasmussen Hmm, not much in the way of repro steps, just added the spacer to a vertical nav block with this branch checked out. Seems the same for Twenty Twenty and Twenty Twenty One (and also the same with theme styles disabled in the editor):
I also notice if you switch between horizontal and vertical nav menus, the spacer seems to retain the width from when it was horizontal. And some Just to check if your styles have rebuilt properly, is your |
|
Okay I was able to reproduce! Thank you. Marcus don't merge this until I have a chance to look at it :D |
|
Okay I restored a property which makes it work for me:
Marcus please test that alignments left right and center as well as the spacer in the vertical nav block work for you also. |
|
This all tested well for me 👍 |
Description
Closes #29459
How has this been tested?
Screenshots
See comments below.