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(app): scroll to top of screen whenever route is changed #16230

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

smb2268
Copy link
Contributor

@smb2268 smb2268 commented Sep 10, 2024

fix RQA-3153, RQA-3139

Overview

Currently when loading a scrollable page on the app, the scroll position from the previous page is maintained so we aren't always dropped on the top of a newly loaded page. It may be worth investigating a better approach down the line so that this automatic scroll doesn't make the scrollbar show up, but for now I've implemented a scrollTo the top of the screen at the top level of the app whenever the route changes.

Test Plan and Hands on Testing

Move between scrollable screens (settings, and protocols and quick transfer if you have more than 5 loaded) and see that you are scrolled to the top of the newly rendered page

Screen.Recording.2024-09-10.at.1.13.34.PM.mov

Changelog

Whenever the current path changes, scroll to the top of the ODD window

Review requests

Let me know if you think there's a better way to do this! This is what I came up with after a day or so of playing around with scrolling.

Risk assessment

Low

@smb2268 smb2268 self-assigned this Sep 10, 2024
@smb2268 smb2268 requested a review from a team as a code owner September 10, 2024 19:25
@smb2268 smb2268 requested review from brenthagen, koji, ncdiehl11, mjhuff and a team and removed request for a team September 10, 2024 19:25
Copy link
Contributor

@brenthagen brenthagen left a comment

Choose a reason for hiding this comment

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

works as expected, no additional rerenders ✔️

Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

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

Is it possible to useMatch on the QT route and just conditionally run this if the page is QT?

If we want this behavior generally, I guess we could blacklist routes...I'm just trying to think of a way to get the scrollbar not to appear when we go to something like /setup

@smb2268
Copy link
Contributor Author

smb2268 commented Sep 10, 2024

Can you useMatch on the QT route and just conditionally run this if the page is QT?

If we want this behavior generally, I guess we could blacklist routes...I'm just trying to think of a way to get the scrollbar not to appear when we go to something like /setup

@mjhuff we're currently seeing this issue across all scrollable pages, so that includes /robot-settings, /protocols, and now /quick-transfers. I'm planning to make a follow up ticket to try to figure out why the scroll position isn't 0 after a route change even when you were scrolled to the top of the previous page but since I don't think this issue is new, design agreed that the scrollbar appearing temporarily is an okay interim fix.

Is /setup scrollable? And without this change, do we always land on the top? I haven't seen the scrollbar appear on any routes that aren't scrollable, like /instruments or an empty /protocols state

@mjhuff
Copy link
Contributor

mjhuff commented Sep 10, 2024

Is /setup scrollable? And without this change, do we always land on the top? I haven't seen the scrollbar appear on any routes that aren't scrollable, like /instruments or an empty /protocols state

Ah gotcha. No, /setup shouldn't be scrollable, but it looks like the scroll appears briefly in the video you linked. IDK if this is really a problem, though. We do always land on the top of that page, since it's not supposed to be scrollable.

Anyway, don't let my comments block you from merging!

@smb2268 smb2268 merged commit 9d9c360 into chore_release-8.0.0 Sep 10, 2024
20 checks passed
@smb2268 smb2268 deleted the app_fix-scroll-position branch September 10, 2024 20:54
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.

3 participants