Fix submenu hover issue. #31195
Merged
Fix submenu hover issue. #31195
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -6 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
9 tasks
getdave
requested changes
Apr 27, 2021
There was a problem hiding this comment.
I tested this and I saw:
- Hover issue is now fixed.
- Menu items still have very large spacing around them.
Screen.Capture.on.2021-04-27.at.10-29-50.mp4
I think the issue with the spacing is that each item has x2 padding applied:
- Padding on the
<li> - Padding on the
<a>.
This doesn't match up with your preview so I assume this is either Theme related or it's a bug. I was using TT1 (standard edition).
Unrelated: I found the cursor changing between text selection and pointer as I mouse between different menu items to be rather "noisy". I wonder whether having it remain as "pointer" and then only once the menu item is active would it change to text selection cursor?
| @@ -220,43 +220,39 @@ | |||
| } | |||
| } | |||
|
|
|||
| // Margins in submenus. | |||
| // Margins in all submenus. | |||
There was a problem hiding this comment.
Is it margins now or just "spacing"?
|
Excellent review, thank you, let me take a stab.
Can you elaborate on this one? Do you mean when the mouse goes from menu item block to space between blocks to menu item block again? Or are you referring to the fact that the editable field of a completed menu item invokes the text selection cursor when hovered? If the latter, I think that's an issue that is important to look at separately, as it potentially affects all blocks, including Buttons. |
jasmussen
force-pushed
the
fix/navigation-menu-issue
branch
from
72ed134
to
0c9c710
Compare
|
Great catch, thank you. Weird how that surfaced here, but should be fixed:
|
This 👍 and definitely not one for this PR. As you move your cursor over a
I might be wrong though because that might break the "make it obvious it's directly editable" paradigm. I just felt it was a little jarring seeing the cursor change all the time. |
getdave
previously requested changes
Apr 27, 2021
There was a problem hiding this comment.
Sorry to say but it looks like we might now be in a worse situation because now the margins that apply to [data-block] are being inherited on the core/navigation-link block and causing huge gaps.
I'm not sure what Theme you are on but I'm on latest TT1 that comes with the wp-env environment.
|
Oh no! Ack, that's TT1 doing it wrong, meaning I need to override and strengthen the CSS rules. Thanks for the reproduction steps, will push a fix. |
jasmussen
force-pushed
the
fix/navigation-menu-issue
branch
from
68f9528
to
7878901
Compare
|
Alright, what a headache, but thanks again for testing and helping me catch this. The original approach, give or take, worked reasonably well for normal block themes which don't excessivley style the canvas:
But TT1, the non block version as you were testing in, was severely broken:
The problem appears to be that TT1 styles the inner padding of menu items using a CSS variable and high specificity. But it does so based on older markup, is unaware of the Page List block, and it does so with a smaller menu item hit area. In 5a5bcbb, I added CSS to explicitly override that:
I believe we should be good now, and it's almost even more important to land a fix soon, AND to test in a wide range of themes, including both normal and block themes. |
vdwijngaert
approved these changes
Apr 28, 2021
|
Thanks so much! @getdave does it address your change requests? |
|
I think Dave is AFK today. @georgeh my friend, if you have time to test this one with TwentyTwentyOne and provide a thumbs up, I think two green sanity checks would provide a nice context for dismissing Dave's review, so we can land this one before the plugin gets built later today. |
georgeh
approved these changes
Apr 28, 2021
|
Thanks for the reviews, Dave, Koen and George. I will land this one so we can build a stable plugin, and I will be on the lookout for any followups I need to address. |
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is a small followup to #30805, which refactored navigation menu margins and paddings. Turned out that margins weren't a good choice for submenu items:
This PR realizes that mistake, and that submenus since they're already inside a box, can be spaced with paddings instead of margins, and look the same, and so it does just that, also fixing the hover issue:
How has this been tested?
Test a navigation menu with no background, then hover submenus.
Checklist:
*.native.jsfiles for terms that need renaming or removal).