Sticky Position: Fix top position while logged in on mobile #47665
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
andrewserong
added
[Type] Bug
andrewserong
requested review from
spacedmonkey,
ellatrix and
ajitbohra
as code owners
|
Just pinging a few people involved with the sticky position support for feedback on the approach in this PR. Happy for any other ideas if this looks off / if there are concerns about adding to the Group block's CSS! CC: @jameskoster @richtabor @apeatling @tellthemachines @jasmussen |
|
Size Change: +340 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This is working well for me, and I'm not against adding the semantic classname for consistency with the layout support, but I'm wondering if it might not be more future proof -considering we may have other position types with similar problems and we'll almost certainly want to apply positioning to more block types -
to instead define a --wp-admin--admin-bar--offset (or something) variable, setting its value to --wp-admin--admin-bar--height only above a certain breakpoint, and put it perhaps in the common file, and then we can use that new variable in the position logic instead of --wp-admin--admin-bar--height?
Oh, I like that idea, that would better contain the logic if we assign the offset there and reference the separate variable like you mention. I'll have a play and see how it looks... |
|
I've pushed an update (6f72dd0) that does the following:
Overall, I think that's a nice improvement over including the CSS in the group block, Happy to make any further changes, though, I don't feel very strongly about the choices I just made 😄 |
|
Flaky tests detected in 52bd2b3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4071771777
|
tellthemachines
approved these changes
Feb 2, 2023
There was a problem hiding this comment.
Looks good! In my previous comment I was thinking that by setting a new custom property we could avoid adding the classname altogether for now (I'm fine with adding it, but at this point in time it's just more stuff to backport). We don't really need to scope the property because it's not being used anywhere else (and it would be fine if it where, because its behaviour just reproduces the admin bar). But I'm not too concerned either way, and people do tend to appreciate semantic classnames 😄
I'm not too concerned, either, but @carolinan made a good point that folks do appreciate semantic classnames, and if it makes it easier for themes to override the rules if they want to do their own thing, then I think I lean slightly toward adding them in. Let's see if anyone has strong opinions on it overnight 🙂 |
carolinan
approved these changes
Feb 2, 2023
|
I think we should keep the class name, unless we think that new settings means that we will need it to be even more specific in the future. Yes people often want to add bottom borders, shadows, animations or reduce an items height etc when it is stickied. |
|
Thanks for confirming @carolinan (and the reviews, both of you!) I'm wrapping up for the day now, if there are no objections from anyone else overnight, I'll merge this in tomorrow morning my time 🙂 |
andrewserong
deleted the
fix/sticky-position-styling-while-logged-in-on-mobile
branch
andrewserong
added
Backport to WP 6.7 Beta/RC
|
@ntsekouras @Mamaduka is this okay to be cherry picked in for 6.2? (It's a bug fix for a feature I'm in the process of backporting, so it'd be great for this one to make it in if we can). Just thought I'd confirm that adding the |
|
@andrewserong, sure 👍 |
Mamaduka
pushed a commit
that referenced
this pull request
Feb 3, 2023
* Sticky Position: Fix top position while logged in on mobile * Clarify comment * Ensure classname is output in editor, too * Fix linting issue * Move CSS to common.scss, decouple from Group block * Use same selector inside the media query Co-authored-by: Carolina Nymark <hi@themesbycarolina.com>
|
I just cherry-picked this PR to the release/15.1 branch to get it included in the next release: a817302 |
Mamaduka
removed
Backport to Gutenberg RC
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 #47566, part of #47043, following on from #46142
Fix the issue where the top position for sticky positioned group blocks is incorrect while logged in on mobile. This is achieved by adding a classname to the position markup, and a CSS rule to the common CSS to reset the admin height CSS variable within the scope of the sticky position classname.
Kudos @carolinan for opening the issue and the suggestion for the fix.
Why?
As reported in #47566, the logged in CSS for the admin bar has it absolutely positioned to the top of the page within the mobile breakpoint, rather than fixed to the top of the page. For viewports larger than
600pxthe existing calculation to offset the area of the admin bar works correctly, however for viewports smaller than600pxthe sticky position output resulted in the sticky block being too low (not flush with the top of the page).How?
is-position-sticky).Testing Instructions
A question for the reviewer: is there a better place to put the override CSS? Does the solution here seem reasonable enough for the time-being? I'd love it if there was a way to avoid introducing this media query, but I couldn't think of an alternative 🤔
Screenshots or screencast