Navigation: Try making it possible for themes to have the X overlay the = icon #43576
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
jasmussen
added
[Type] Enhancement
|
Size Change: -1.94 kB (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
mikachan
reviewed
Aug 25, 2022
There was a problem hiding this comment.
Confirming that this has been tested with Twenty Twenty-Three in a PR here. It's working well, especially using the wide align on larger resolutions 👍
I've also tested using Remote, which doesn't opt into the root padding setting. This theme uses the alignment styles from Twenty Twenty-Two (before we had root padding options in GB), so I'm not sure how valuable this test is. I needed to change the global padding, set in Remote using a settings.custom.spacing.outer variable, to match the default 2rem. After making this change, this PR works really well.
Another good theme to test is probably Block Canvas, as it's a simple starter that doesn't opt into root padding. One difference I noticed here was that, by default, this theme has the site title and tagline stacked on the left of the header, and the menu overlay icon on the right. As the left content is taller than the right, and the row block is vertically centered, then the overlay icon sits lower down:
Screen.Recording.2022-08-25.at.11.54.04.mov
If I change the vertical alignment to the top, the icon positions match again:
Screen.Recording.2022-08-25.at.11.56.38.mov
This is no different from the current behaviour with this theme, I just wanted to point out the difference in layout. There's a PR here for both of these themes, for easy testing.
I think this PR is definitely a positive step forward. It's working great for the common use case of a top-right aligned menu, and it's neat that it uses the root padding values where available.
jameskoster
reviewed
Aug 26, 2022
There was a problem hiding this comment.
It works well when the Navigation containers have no horizontal padding:
I didn't expect the icons to be aligned after adding some padding to the group containing the Navigation block. But I also didn't expect the padding of the overlay to be affected, which seems to be the case:
Note how the X is positioned differently to the previous gif. I don't think it's a big deal, but I'm not sure if it's intentional, or why it's happening, so figured it was worth noting.
|
Curious, I can't repro that. Here's a bunch of padding on the group:
It's only offset what I would expect it to be, and matching the extra padding value. Can you web-inspect and see what's going on there? The overlay should have these values: And those values should match what's in the root:
|
jameskoster
approved these changes
Aug 26, 2022
|
Let's try this. It's better than the previous hardcoded 2 rem. |
reid
added a commit
to AwakeningSV/wordpress
that referenced
this pull request
Sep 17, 2022
Fix useRootPaddingAwareAlignments by using an updated schema which has the correct location for this setting. As of this writing, this blog post does not contain the correct example code for this setting. The setting must be inside the settings object and not at the root: https://make.wordpress.org/themes/2022/09/07/full-width-blocks-and-root-padding-in-wordpress-6-1/ The schema is correct and was fixed here: WordPress/gutenberg#43624 The navigation block uses this setting to correctly render the close icon: WordPress/gutenberg#43576
82 tasks
15 tasks
38 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.
What?
Fixes #43523, even if more work can be done in the future (for an animated icon or otherwise).
When the navigation block is set to collapse into a modal overlay, right now it is impossible for a theme to perfectly ensure that the X to close perfectly overlaps the = menu icon. This is trunk:
Simply to ensure a consistent experience, this should be possible. This PR makes it possible under these circumstances:
"useRootPaddingAwareAlignments": truein theme.json under settings, and paddings to go with that either in global styles or under styles > spacing.How?
The code to accomplish this should work, as it simply taps into the root padding values if they are there, but falls back to base values if they are not. I'd appreciate a sanity check on this approach, however, and alternatives if there are better ones.
Testing Instructions
This one is a bit tricky to test as the theme has to use the root paddings. I believe that Archeo does this, so that could be a theme to test.
Combine that with ensuring the navigation collapses to a burger, and is top-right & justified right in the header template part in the site editor. Then verify the X overlaps the = at all times.
Also test with a theme that does not opt into root paddings, and check that the fallback values work as intended.