Polish responsive navigation modal, inherit justifications, fix submenu direction #35077
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: +563 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
|
I'm noticing that spacing looks a bit funky with Page List, and subpages, both in TT1 and T19; here's TT1.
|
|
Nice this fixes justification for the vertical variant! One thing I noticed was that space between was not respected when the menu is set to responsive:
|
jasmussen
force-pushed
the
fix/navigation-responsive-justification
branch
from
65d746d
to
56a962a
Compare
|
Thanks a ton for testing, both of you. Vicente, it appears the spacing issue with the navigation is due to obsolete margin rules applied by TT1, tracked here: https://core.trac.wordpress.org/ticket/53220 You can see here that the gap is the same:
But the main items have a padding applied by the theme, which interferes with it: The white border around the responsive modal dialog is also from TT1 styling the wrong background color, it appears:
This is also reflected if you choose a different background color:
The white background behind the active menu item is because TT1 also sets the bg color on focus, and in a modal dialog, the first item gets focus as soon as it's opened. There seems to be a legitimate issue with the spacing of items in the "unfolded" style:
I'll see what I can do here. About the TT1 issues: it was built to represent the best WordPress had to offer coinciding with a release. Which meant it did the very best it could with the markup and featureset the navigation block had at the time, which has changed a great deal. I will see if I can set aside some time to submit a patch and fix it at the source. But in the mean time, it's possibly best to test with another theme, possibly "Empty Theme" from this repo. |
|
Thank you Kerry, great catch. Space between should work in Page List again: Here's updated test content in case you need it: |
|
Alright, I believe I fixed the issue with bad spacing for menu items. Before: After:
The issue was related to the refactor to use gap, where submenus still partially used margins. The new version uses a combination to space things out correctly. Here's some test content just for testing all the justifications inside the modal: |
|
Thanks for working on this. I see that now the menu opens to the left if the items are justified right, but the arrows still point to the right: |
|
Also, when using post list and other items in a horizontal menu, it still displays vertically:
The markup for this one is: |
|
Thanks for testing. I'll address the post list. Mind if I ticket or look at the arrow changing direction separately? The current behavior is semi intentional in that it mirrors what MacOS does:
— though I agree it's less obvious here, so worth looking at. |
Yeah that makes perfect sense :) |
|
@jasmussen do you mind rebasing with trunk, we're running into the |
jasmussen
force-pushed
the
fix/navigation-responsive-justification
branch
from
66cd3d6
to
e4c1ecb
Compare
|
Done, thank you for looking! |
gwwar
approved these changes
Sep 28, 2021
There was a problem hiding this comment.
Nice work @jasmussen! Giving tentative approval, but feel free to land when we have tests green.
I verified that alignment and space between still work. I did notice some jumping in edit mode when selecting a block with justify right or space between, but I think that's more minor and can be addressed in a different PR.
|
Thank you! I'll let this one sit until tomorrow and look at merging it then so I can follow up. In the mean time I would welcome any tests and additional reviews. |
|
@jasmussen @gwwar It seems like there was a lot going on here, but absolutely no testing of the navigation editor, and this PR has caused issues with the appearance of items in the navigation editor. There are quite a number of problems now when we factor in the problems also introduced by #35026. In addition to the problem that PR introduced, this one seems to have caused issues with the hover and selected block styles: The border is cut-off, and there's a lot of general glitching when hovering over items (the whole nav block unexpectedly resizing, different borders showing up). I understand that the way the editor's styles work right now isn't ideal, but I don't think this is a valid excuse to bypass testing and fixing issues as you encounter them prior to merging. Both the editor and the block are a shared responsibility for those working on this project, and it'd be great to see more sharing of responsibility in the future when contributing. |
7 tasks
|
Apologies. I included a fix for the hover style in #35234. Can you clarify what you are seeing with regards to the nav block resizing, different borders showing up? I'm seeing the following which seems right to me: |
|
Left a video and comment on #35234. |
Sorry @talldan I usually remember, but I forgot for this round of testing! |
This was referenced Nov 1, 2021
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
Fixes #35040. Fixes and issue where justifications were not inherited in the responsive modal, an issue with hover-opening flickering, and an issue with submenus not opening in the right direction.
This is a big of a big one so could use a great deal of testing in a variety of themes, with a variety of content. Things to look for:
All of that, worth testing with and without responsive navigation toggled on, and with and without a Page List block inside.
How has this been tested?
You can use this testing content:
Screenshots
Before, editor:
Before, frontend:
Before, inside the modal, even though justification was set to right:
After, frontend:
After, submenus when justified right:
After, modal:
Checklist:
*.native.jsfiles for terms that need renaming or removal).