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

fix: fix sticky nav jumping on narrow screen #685

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wa0x6e
Copy link
Contributor

@wa0x6e wa0x6e commented Sep 1, 2024

Summary

The sticky nav has a different position on narrow screen, making it "jump" by one pixel on scroll

Screen Recording 2024-09-01 at 17 15 30

Weirdly, there's 2 different positions (71px and 72px), depending on screen size, even though the topnav always have the same height in all screen

How to test

  1. Go to https://snapshot.box/#/s:safe.eth/proposal/0x2c2ad686e94fd97903de8ee520b90d400c98a1985e31b21416da095b28373f18 on a narrow screen
  2. Scroll down
  3. The sticky nav will move by one pixel

@ChaituVR
Copy link
Member

ChaituVR commented Sep 2, 2024

Not able to reproduce this as well :(

@wa0x6e
Copy link
Contributor Author

wa0x6e commented Sep 2, 2024

  • Resize screen to 900px width
  • Go to top of the page
  • Observe the sticky nav
  • Start scrolling down slowly

Copy link
Member

@Sekhmet Sekhmet left a comment

Choose a reason for hiding this comment

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

71px was used because of this issue, this issue shows up on some devices (Fabien has gift to reproduce it on everything he touches 😆), to repro open proposal page with 200% zoom in devtools and compare.

Screen.Recording.2024-09-02.at.19.00.09.mov

With this PR you can see content that's between Topnav and sticky menu on scroll.

@wa0x6e
Copy link
Contributor Author

wa0x6e commented Sep 3, 2024

71px was used because of this issue, this issue shows up on some devices (Fabien has gift to reproduce it on everything he touches 😆), to repro open proposal page with 200% zoom in devtools and compare.

Screen.Recording.2024-09-02.at.19.00.09.mov
With this PR you can see content that's between Topnav and sticky menu on scroll.

This seems a very special edge case.

I can not test this edge case, due to my screen size not reaching more than 800px wide on a 200% zoom.

We could get rid of this issue by using 71px everywhere, and still have a 72px topnav, making everything overlapping by 1px, but this will just mask the issue.

I will try another solution, by getting rid of all these placement using margin

@Sekhmet
Copy link
Member

Sekhmet commented Sep 3, 2024

I can not test this edge case, due to my screen size not reaching more than 800px wide on a 200% zoom.

You can test it with device simulator:
image

@wa0x6e
Copy link
Contributor Author

wa0x6e commented Sep 4, 2024

I see. In my case, can reproduce it only on a 75% zoom, all other zoom working well. Guess this is some scaling, since by changing the screen resolution, I also got the issue on a 125% zoom.

@wa0x6e
Copy link
Contributor Author

wa0x6e commented Sep 4, 2024

Weirdly, we don't have this issue at all on the space settings page

@wa0x6e
Copy link
Contributor Author

wa0x6e commented Sep 4, 2024

71px was used because of this issue, this issue shows up on some devices (Fabien has gift to reproduce it on everything he touches 😆), to repro open proposal page with 200% zoom in devtools and compare.

Screen.Recording.2024-09-02.at.19.00.09.mov
With this PR you can see content that's between Topnav and sticky menu on scroll.

The quick fix we have now on master (using a mix of 71 and 72x) does not seem to work either on some zoom.

On a wide screen (like 1200px), set the zoom to 75%, and we still see the gap.

And on zoom without dev tools (just zoom the regular page with CMD +/-), we also observe the gap on some zoom settings

Screenshot 2024-09-04 at 14 57 29
Screenshot 2024-09-04 at 14 57 45

@wa0x6e
Copy link
Contributor Author

wa0x6e commented Sep 4, 2024

On this video, I am scrolling down the page (white content background for easier detection)

Loom Message - 4 September 2024 - Watch Video

Notice that the gap is appearing only half of the time, the gap is coming from a rounding error

@wa0x6e
Copy link
Contributor Author

wa0x6e commented Sep 4, 2024

Got more insight on this issue:

  • depending on the scaling and zoom, the top 72px is not exactly 72px, but a float (let's call it X)
  • As you scroll the page, browser recompute, and X may change by a fraction

Look at the video (you may need to download and zoom it): as it's scrolling down, the sticky nav is moving up and down by half a pixel on each scroll (which is why we see the gap only half the time). This half pixel up/down move also make the sticky nav off pixel, making the text slightly blurred.

Screen.Recording.2024-09-04.at.16.53.42.mp4

This is technically a browser issue, and any fix will be hacky

@Sekhmet
Copy link
Member

Sekhmet commented Sep 9, 2024

I think we should prioritise it working on 200% zoom to emulate HiDPI mobile screens, otherwise we would have mobile devices with this issue. Does using 71px achieve that?

@wa0x6e
Copy link
Contributor Author

wa0x6e commented Sep 9, 2024

Always using 71px (so losing 1px by overlapping elements) works.

This issue technically also happens everywhere we're using 72px, like the sidebar, or sticky nav on other page.

It does not happen on the space settings page though, and still trying to figure out why, and if we could reproduce and use this method everywhere.

@Sekhmet
Copy link
Member

Sekhmet commented Sep 11, 2024

@bonustrack do you reproduce 1px gap between topnav and sticky elements on your device here?

@bonustrack
Copy link
Member

On this PR I can't see the 1 pixel bug, but I noticed on votes page the first manu bottom border is missing.

Screenshot_20240913_002344_Chrome

@bonustrack
Copy link
Member

Ideally we should move this logic and style into a component that we can reuse and style without changing multiple views. We could also add gesture slide left and right to move between tabs.

@bonustrack
Copy link
Member

And the 1px bug is something you can reproduce on Chrome mobile, (Android for me, not sure on iOS). But snapshot.box don't have any 1px bug left I believe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants