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

Scroll entire page instead of content pane #2450

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Conversation

david-crespo
Copy link
Collaborator

@david-crespo david-crespo commented Sep 17, 2024

Trying to extract the simple thing out of #2087. It mostly works, but there are still some issues:

  • In theory this change should let us switch back to the <ScrollRestoration /> thing built into RR, but it's not working yet
  • Figuring out little things like full-page layout shift due to modals removing the scroll bar
video of layout shift
2024-09-16-menu-kills-scrollbar.mp4

Copy link

vercel bot commented Sep 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Sep 19, 2024 9:35pm

@david-crespo
Copy link
Collaborator Author

While messing around with converting dropdowns to use Headless instead of Radix (which we want to do anyway), I figured out you can fix the layout shift by making all dropdowns modal={false}. Still need to fix the z-index on the dropdown or the topbar, as shown.

2024-09-17-dropdown-menu-z-bug.mp4

@david-crespo
Copy link
Collaborator Author

Pagination not landing at the bottom is the only remaining issue that I'm aware of. I had to switch back to our custom scroll restoration hook because I couldn't get the RR one to work unless I wrap their window.scrollTo call in a setTimeout, which means it's running too early (theirs runs in a layout effect and ours runs in a regular effect).

image

@benjaminleonard
Copy link
Contributor

Are we running into this because the query has not come back yet, the table not populated and therefore the page is not long enough to scroll to when the scroll restoration has run?

@david-crespo
Copy link
Collaborator Author

I think I assumed that was the case, and I probably did check the scrollable height, but I don’t remember whether I printed the entire DOM at the time scrollTo is called to confirm it.

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.

2 participants