- Notifications
You must be signed in to change notification settings - Fork1k
chore: improve scroll behavior of DashboardLayout wrapped pages#19396
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
lgtm! this is a precarious enough change I'd like to get a second pair of eyes on it tho, maybe@Parkreiner?
Agreed, and if someone has time to pull this down and check it out locally I'd appreciate it too. A weak point of our storybook setup is we don't really test for scrolling content as often as we should |
I ran it locally and didn't notice any jank, I'm just paranoid lol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good! I tested as many pages as I could, both with and without the bottom bar, and things seemed to work well
Just had some small nits on the styling
| return( | ||
| <> | ||
| <divclassName="w-full max-w-screen-2xl pb-10"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there any way we can refactor the layouts to bake thepb-10 in by default? I'm worried that it could get missed as we add new page types in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I believe I tried doing that, but the pages aren't set up consistently so it didn't end up being feasible. And making the pages more consistent seemed out of scope for this PR
| if(error||workspacePermissionsQuery.error){ | ||
| return( | ||
| <divcss={{margin:16}}> | ||
| <divclassName="m-4"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Totally okay if this isn't feasible because it would require too many changes elsewhere, but could we update this to use padding instead? I feel like margins are an anti-pattern ~95% of the time
d274f83 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Updates the the
DashboardLayoutto create a singular scroll area between the top nav bar and the deployment banner on the bottom. Also improves the scroll behavior of the org settings pages.CleanShot.2025-08-18.at.13.49.29.mp4