Fix issue with responsive navigation causing wrapping. #35820
Merged
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] Bug
|
Size Change: +149 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
| @@ -230,7 +230,8 @@ | |||
| // Menu items with no background. | |||
| .wp-block-navigation, | |||
| .wp-block-navigation .wp-block-page-list, | |||
| .wp-block-navigation__container { | |||
| .wp-block-navigation__container, | |||
| .wp-block-navigation__responsive-container-content { | |||
| gap: var(--wp--style--block-gap, 2em); | |||
There was a problem hiding this comment.
This is causing an extra separation on mobile, don't know if it's intended:
There was a problem hiding this comment.
Hrm. Good catch, I don't think it is. Let me take a look.
There was a problem hiding this comment.
I'm going to open a PR for the padding issue on the social links, btw
There was a problem hiding this comment.
I'm going to open a PR for the padding issue on the social links, btw
You mean this one, right?
If yes, then the way to fix that is probably to replace a with .wp-block-navigation-item__content, as we've done with all the other instances of the raw anchor link.
There was a problem hiding this comment.
Hmm, this is a little messy:
Essentially we want to treat menu items and social links as if they were spaced with the same gap, as direct descendants of one container rather than each with their own child container.
How would you feel about applying display: contents; on those containers? I feel like it would open a deep rabbit hole, so I'l probably fix the gap as is but it's a bit of a head scratcher.
There was a problem hiding this comment.
mmh, I'm thinking, could we use column-gap in this case instead of gap? Then it only affects them horizontally.
There was a problem hiding this comment.
It wasn't so much the vertical issue, I slipstreamed a fix for that. It was more that we have this structure:
Navigation
Link Container
... menu items
Social Icons Container
... social icons
Essentially I feel like we want to apply gap as if the structure was this:
Navigation
Menu Item
Menu Item
Menu Item
Social Link
Social Link
Social Link
Right now this PR accomplishes that by applying gap to both the root navigation container, the Link container and the Social Icons container. Just wondering whether in the future/a separate exploration, there's a better way 🤔
There was a problem hiding this comment.
I see what you mean, let me test this a bit
7 tasks
|
Superb review, thank you. I pushed some fixes to:
Things are now looking good to me: |
|
I'm breaking this a bit more, I'm sorry! I'm using this markup: On desktop this works like a charm: Before:
vs After: On mobile though the spacing is still a bit off when I'm adding the search block. Before:
After:
I agree with what you say about treating social and menu links as if they were inside the same container, but I don't think that applies to other blocks that may be inside the navigation. This is a tricky one! |
|
This is excellent, thank you for testing. I'm wondering what to fix here vs. in #35823, where I'm tackling a bit of the same issue. |
|
Let me take a quick look at that search+social links thing, I need to rebase the other one anyway! Thank you. |
|
Solid debugging. I removed the rule to unset the gap inside the open overlay menu after all, because the thing that causes problematic spacing is the margin: And so there's a little extra space now:
But I believe I can/should fix that in #35823. The rest also looks good:
|
Yeah, looks like getting rid of the margins is more consistent too with how we are aiming to use gap. Works for me and the tests look fine! |
MaggieCabrera
approved these changes
Oct 21, 2021
7 tasks
fullofcaffeine
added a commit
that referenced
this pull request
Oct 21, 2021
* trunk: (494 commits) remove consecutive rc warning (#35855) Update Changelog for 11.8.0-rc.2 Bump plugin version to 11.8.0-rc.2 [RNMobile] Disable React Native E2E Tests (iOS) (#35844) Add section about using the schema during development (#35835) Add a method to disable auto-accepting dialogs (#35828) Wrap NavigationContainer with SafeAreaView. (#35570) Update Appium to 1.22.0 (#35829) Post Comment: Handle the case where a comment does not exist (#35810) Clear selected block when clicking on the gray background (#35816) Post excerpt: Don't print the wrapper when there is no excerpt (#35749) [Block] Navigation: Fix padding for social links on mobile (#35824) Fix issue with responsive navigation causing wrapping. (#35820) [Block Editor]: Fix displaying only `none` alignment option (#35822) Add API to access global settings, styles, and stylesheet (#34843) Mobile Release v1.64.1 (#35804) Add resizer to template part focus mode (#35728) Update Changelog for 11.7.1 Gallery block: Only show the gallery upload error message if mixed multiple files uploaded (#35790) Update Changelog for 11.8.0-rc.1 ...
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 #35549.
When the mobile responsive overlay menu was enabled, subsequent child containers such as social links would wrap on a new line:
This PR fixes that:
How has this been tested?
Here's some test content:
Insert a navigation menu with both a few items, and some social links. Enable responsiveness ("mobile"), and verify the layout doesn't change. Verify also for the vertical version. And check that the overlay still looks good.
Checklist:
*.native.jsfiles for terms that need renaming or removal).