-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Navigation: Try unifying submenu arrow positioning. #37003
Conversation
Size Change: +2.5 kB (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
@tellthemachines I know how busy you are, but if you have time, I'd appreciate just a high level sanity check of the code here, and whether it's something we can land for 5.9. |
f16c662
to
b57a227
Compare
Fixes #38204. I would like a confidence boost and good testing of the code, but since it fixes an issue, I think we should revisit this one. |
b57a227
to
bbeabf1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bbeabf1
to
b915e07
Compare
If you have time to check the code of this one again, I'd be grateful. It tests well for me, feels safe, and I'd like to land it. But because it touches some of the markup, I'd love a sanity check just so there isn't anything I've missed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, the arrows are looking much better! Just a couple comments below; mainly we need to make the change for page list in the front end and then should be good to go 🚀
<span className="wp-block-page-list__submenu-icon wp-block-navigation__submenu-icon"> | ||
<ItemSubmenuIcon /> | ||
</span> | ||
</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change also needs to be done for the page list front end, here. As it stands there's still a diff in the front end between open on hover and click with page list submenus:
(top one opens on hover, bottom on click).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I wanted to note that I missed this because it appears I can't currently a navigation menu with only a page list to open submenus on click in the interface. As of #36826, we currently hide those options unless the menu hasSubmenus
:
It seems like we should change that to simply just show those options if the menu hasPageList
or something along those lines, it doesn't seem worth it to check if the pagelist has submenus. CC: @vcanales
I was able to check the box in the code editor though, so I'll take a stab at addressing the markup issue now.
@@ -245,7 +257,8 @@ $navigation-icon-size: 24px; | |||
} | |||
|
|||
// Show submenus on click. | |||
.wp-block-navigation-submenu__toggle[aria-expanded="true"] + .wp-block-navigation__submenu-container { | |||
.wp-block-navigation-submenu__toggle[aria-expanded="true"] + .wp-block-navigation__submenu-container, // Without submenu icon showing. | |||
.wp-block-navigation-submenu__toggle[aria-expanded="true"] + .wp-block-navigation__submenu-icon + .wp-block-navigation__submenu-container { // With submenu icon showing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to use ~
instead of + .wp-block-navigation__submenu-icon +
here.
265f2cd
to
90ad80e
Compare
Thank you for the excellent review. Per your advice I pushed 953e0c5 which appears to fix the issue also for Page List: |
I found that also 😅 #38439 |
With this change the icon should always be outside the I think that's fine, but you raise a good point about the behaviour difference: it would be better if, with "open on click" set, clicking on the icon also opened the submenu. We could fix that by giving the button a bit of right padding, and absolutely positioning the icon span over it, with |
Hmm that's an excellent call. I can confirm that I can click on the arrow, just like the text, to open menus in trunk, but not in this one. That does seem worth fixing. I need to work on some other things this morning, but I'm going to add an "in progress" label and see what I can do about fixing that. Thank you both! |
953e0c5
to
ddf412a
Compare
ddf412a
to
c1601b0
Compare
As I started work on changing the shape of the button to make a faux clickable area on the chevron, it didn't feel right. But the changeset ended up being okay, I think, it works well, and honestly it comes down to choosing whether to have CSS complexity handling two different positions of the icon in the markup, or handling a little padding math. Right at this point I'm leaning towards the padding math for the simplicity. But I'd appreciate another set of eyes. I also noticed an extra span being output for the Page List when set to open submenus on hover; in that situation, the Here it is working: |
width: 0.6em; | ||
height: 0.6em; | ||
margin-left: 0.25em; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it comes down to choosing whether to have CSS complexity handling two different positions of the icon in the markup, or handling a little padding math.
I'm happy with either; the icon has no semantics attached so it doesn't really make a difference from that perspective.
I pushed a small change to the page list markup on the editor side, as the hover version of the icon was being rendered outside the button; I updated it to match the front end. With this I think we're good to go 🚀
} | ||
|
||
// For hover-based menus, this class is duplicated for a button. | ||
.wp-block-navigation__submenu-icon .wp-block-navigation__submenu-icon { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed? I notice you removed the extra span here (which wasn't meant to be there, so thanks for fixing 😄 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wildly excellent eye there. I started writing the CSS rule first, then figured the better fix was to remove the span, then forgot to remove the CSS 🙈
Thank you, removed!
Thanks so much for all the help with this one. I think I'll land it, as it fixes an open issue, and it simplifies many more styling issues across hover and click, and introducing just that little extra padding rule for the hover situation. I think it's worthwhile balance. I also gave it a final sanity check, and it's look good to me: |
Description
This PR is intentionally a draft, because I have this feeling I'm messing with a delicate structure, and so I want to make sure this is right before it comes out of the draft state.The challenge to solve is to ensure that the dropdown icon looks and is positioned the same inside the editor and on the frontend, regardless of whether you have hover or click enabled. Right now they are not. Editor:
Frontend:
The cause is that the markup is slightly different. In the editor, with both label and icon are always children of the
wp-block-navigation-item__content
element:On the frontend, this varies depending on click and hover, but it's sometimes like so:
This PR changes the structure so the icon is always outside of the
wp-block-navigation-item__content
element. Editor:Front:
How has this been tested?
Insert navigation menus with submenus, one that opens on click, one that opens on hover, and verify the icons look identical between them, and with the frontend.
The positioning on the frontend can be tricky to see at small sizes, so it can help to bump the font size. Vertically the chevron should be pushed down ever so slightly.
Checklist:
*.native.js
files for terms that need renaming or removal).