Navigation block: fix padding on mobile overlay when global padding is 0 #53725
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
MaggieCabrera
added
[Type] Bug
|
@jasmussen I'm trying this, and it doesn't work after building the CSS, but if I write the same CSS manually in the browser inspector, it works as intended!!! I don't understand what's going on here. Also, we need to consider that this solution will make any themes that have a padding set that is more than 0 but less than 2rem to jump between closed/open. It's not ideal but I think it's better than what we have and we have the expectation of a better solution to come with #43852 |
|
Size Change: +43 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
It fails if the padding is declared as 0 without any unit, then the max() doesn't know what to do. And the compiler is merging the 4 padding values into the shorthand so having one wrong value breaks the whole thing :( |
If that's the SCSS thing, then there should be a way around that. For example if I want a variable explicitly output so that Let me take a look. |
|
Is it safe to assume that theme authors should add units for this to work? I can make the fix for Blue note, I'm not sure how many other themes would make the same mistake of adding the unitless 0 |
|
I think I may have a fix in https://codepen.io/joen/pen/RwEbgQv?editors=1100 Specifically, it seems like |
It breaks for --global-padding: 4rem; |
|
Oh right. 🤔 Maybe there is no obvious fix other than addressing that theme.json styleable overlay. |
I think this here is the best we can do for now! |
|
I'm gonna go for a bit but I'll be back and write a proper description and test instructions |
MaggieCabrera
requested review from
draganescu,
scruffian,
getdave,
jeryj and
ajlende
|
I added a commit to use clamp. This means that we can set a min and max, but also allow the size to be the correct one if that's an option. Feel free to remove it if you don't like that option. WRT the |
|
https://www.oddbird.net/2022/08/04/zero-units/ I’m not sure adding the unit is a good idea |
|
In that case we should probably try to educate peopler about why they should use |
This was referenced Sep 8, 2023
tellthemachines
approved these changes
Sep 20, 2023
There was a problem hiding this comment.
Code looks good and the change is working well for me!
One thing I noticed in testing (unrelated to this PR) is that a 46px margin-top is added to wp-block-navigation__responsive-dialog on the front end when the admin bar is detected, but the nav overlay hides the admin bar. Might be that z-indexes were messed with at some point? I'm guessing the intention was for the admin bar to show when the overlay is open?
tellthemachines
force-pushed
the
fix/nav-overlay-padding
branch
from
ad50606 to
e06291e
Compare
|
The e2e failure looks legit and I can reproduce it locally on this branch, but the behaviour tested hasn't changed from trunk to this branch. I suspect something in the spacing change causes hover on the page link button to be activated, and the hover color is being read as the actual link color 🤔 Edit: failure artifact confirms the problem is hover style:
|
This comment was marked as resolved.
This comment was marked as resolved.
getdave
force-pushed
the
fix/nav-overlay-padding
branch
from
e06291e to
6f92e28
Compare
|
Ok I believe I've fixed this. The issue was that when the mobile overlay menu was toggled open the mouse pointer was sitting directly on top of one of the links in the overlay. This caused the I've fixed this by shifting the mouse down and to the right by a large value in order to ensure that the mouse cannot accidentally trigger any state changes. |
Wow, nice detective skills there! Thank you for picking this one up |
|
Auto merging once tests pass 👍 |
MaggieCabrera
added
the
Backport to Gutenberg RC
|
I added the backport label since this is affecting TT2 and TT4 @mikachan |
|
Given that there are no PHP changes in this PR these constantly failing PHP tests seem to be anomalous. Agreed? Re-running again in the hope that they pass this time. There is no error, just that the test fail to run 🤦 |
mikachan
pushed a commit
that referenced
this pull request
Sep 26, 2023
…s 0 (#53725) * force min value for padding to be 2rem * fallback for when the css variables are not defined * Allow the padding to be smaller than 2rem * Add fix to avoid trigger hover state on links when overlay opens --------- Co-authored-by: scruffian <[email protected]> Co-authored-by: Dave Smith <[email protected]>
|
I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: 7648f9b |
mikachan
removed
the
Backport to Gutenberg RC
mikachan
pushed a commit
that referenced
this pull request
Sep 26, 2023
…s 0 (#53725) * force min value for padding to be 2rem * fallback for when the css variables are not defined * Allow the padding to be smaller than 2rem * Add fix to avoid trigger hover state on links when overlay opens --------- Co-authored-by: scruffian <[email protected]> Co-authored-by: Dave Smith <[email protected]>
Mamaduka
added a commit
that referenced
this pull request
Sep 26, 2023
* use the wporg cdn (#54795) * core-data: Fix nested property access with undefined name (#54790) * core-data: Fix nested property access with undefined name * Add a unit test * Social Links: add X (#54092) * Social Links: add X Fixes #53223 * Add Twitter keyword to variation This will allow people to find the new icon when searching for Twitter. See #53223 (comment) * Reorder links alphabetically Co-authored-by: Aki Hamano <[email protected]> * No need for a capital letter Co-authored-by: Aki Hamano <[email protected]> * Fix svg attributes See #54092 (comment) Co-authored-by: Rich Tabor <[email protected]> * Remove "icon" Co-authored-by: Nick Diego <[email protected]> * Update X icon path See #54092 (comment) See #54092 (comment) --------- Co-authored-by: Aki Hamano <[email protected]> Co-authored-by: Rich Tabor <[email protected]> Co-authored-by: Nick Diego <[email protected]> * remove font files created by tests after tests run (#54771) * Check that new pattern is synced before replacing existing blocks with a synced pattern (#54804) * Patterns: Improve sentence case consistency of labels and notices (#54807) * Navigation block: fix padding on mobile overlay when global padding is 0 (#53725) * force min value for padding to be 2rem * fallback for when the css variables are not defined * Allow the padding to be smaller than 2rem * Add fix to avoid trigger hover state on links when overlay opens --------- Co-authored-by: scruffian <[email protected]> Co-authored-by: Dave Smith <[email protected]> * Always show the total number of patterns even with only one page (#54813) * Always show the total number of patterns even with only one page * Add to explorer too * Hide total number of 0 * Font Library: Avoid rendering Font Library UI outside Gutenberg plugin (#54830) * Remove action to fix tests. (#54806) * Conditionally remove deprecated 'print_emoji_styles' (#54828) * Fix Performance tests * Fix global styles revision --------- Co-authored-by: Matias Benedetto <[email protected]> Co-authored-by: Marin Atanasov <[email protected]> Co-authored-by: Jeremy Herve <[email protected]> Co-authored-by: Aki Hamano <[email protected]> Co-authored-by: Rich Tabor <[email protected]> Co-authored-by: Nick Diego <[email protected]> Co-authored-by: Glen Davies <[email protected]> Co-authored-by: Aaron Robertshaw <[email protected]> Co-authored-by: Maggie <[email protected]> Co-authored-by: scruffian <[email protected]> Co-authored-by: Dave Smith <[email protected]> Co-authored-by: Kai Hao <[email protected]> Co-authored-by: Jonny Harris <[email protected]> Co-authored-by: George Mamadashvili <[email protected]>
6 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?
This PR expands on the fix made by #43576 to cover the cases when themes don't declare a global top padding for their site (or they declare it as 0px). This makes sure that the padding value is not 0.
Fixes #52233
Why?
The original fix was adding a 2rem fallback if the global padding is not set by the theme. That fallback is probably only working for classic themes, since when block themes don't declare a global padding, the value of the CSS variable resolves to 0px, causing this issue on the overlay menu:
There is a use case for themes with no global padding-top when the theme wants to have a different color on the header than the body, where the theme uses group padding instead to separate the menu from the top of the viewport.
How?
Using
max(), we make sure that the highest value of the pair is always selected, so if the global padding is 0, it will default to 2rem instead.There are two caveats to this implementation:
max() needs the values to have units, else they won't work.Testing Instructions
In your theme.json file, define the global padding as either
or
Check the frontend both on a logged in and logged out state, and notice that the burger icon is separated 2rem from the top of the viewport.
Also, check that if the value for the global padding is bigger than 2rem, that's the value that's been applied for the overlay too