Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refresh: Fix nav sticky scrolling styling/condition #15673

Merged
merged 3 commits into from
Dec 12, 2024

Conversation

janbrasna
Copy link
Contributor

@janbrasna janbrasna commented Dec 4, 2024

One-line summary

Only applies "scrolling" or "sticky" decoration once the navbar actually settles in its sticky position.

Significant changes and points to review

Not sure if this is the least fragile approach, but felt like the most obvious one.
Be skeptical, please. Would love to learn any better and simpler way!

Issue / Bugzilla link

Fixes #15619

Testing

Yes, please. Things like pinch zooming, scrolling or dragging beyond the viewport, checking any motion a11y preference is still being honored etc.

// add styling for when scrolling the viewport
if (window.scrollY > 0) {
// add styling for when scrolling the viewport and nav is already sticking
if (window.scrollY > 0 && _navElem.getBoundingClientRect().top == 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. I'd add some position tolerance instead of hard y===0, given the potential subpixel precision of this or some "rubberband" overscroll etc:

Suggested change
if (window.scrollY > 0 && _navElem.getBoundingClientRect().top == 0) {
if (window.scrollY > 0 && _navElem.getBoundingClientRect().top < 1) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been considering Intersection Observers but that felt like needing a little different approach for most of the MzpNavigation.*Sticky logic, so this was to minimize the change surface. Observer API would be a better approach generally, so if this feels wonky at times that might be the next iteration.

@janbrasna janbrasna marked this pull request as ready for review December 4, 2024 22:31
@janbrasna janbrasna requested a review from a team as a code owner December 4, 2024 22:31
@stephaniehobson stephaniehobson added Needs Review Awaiting code review Review: XS Code review time: up to 30min labels Dec 5, 2024
@stephaniehobson stephaniehobson self-assigned this Dec 12, 2024
Copy link
Contributor

@stephaniehobson stephaniehobson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple but effective. Holds up on iPad/Safari, iPhone/Safari, Samsung/Chome, Mac/Chrome, Mac/Firefox, Mac/Safari, Windows/Edge.

R+

@stephaniehobson stephaniehobson merged commit 11c0669 into mozilla:main Dec 12, 2024
5 checks passed
@janbrasna janbrasna deleted the fix/m24-sticky-nav-styling branch December 12, 2024 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review Awaiting code review Review: XS Code review time: up to 30min
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refresh: Nav drop shadow breifly obscures pencil banner
2 participants