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

Remove scroll from whole app to individual scrollable components #5070

Open
nabramow opened this issue Oct 28, 2024 · 3 comments
Open

Remove scroll from whole app to individual scrollable components #5070

nabramow opened this issue Oct 28, 2024 · 3 comments
Labels
1.topic frontend 3.skill good first issue improvement Not really a feature but also not a bug

Comments

@nabramow
Copy link
Contributor

nabramow commented Oct 28, 2024

We have scroll set to the whole app rather than just in the scrollable components. This means even the nav bar scrolls, which is not what we want.

See what I mean on desktop here:

ScreenRecording2024-10-27at6 46 19PM-ezgif com-video-to-gif-converter

On mobile here:

ScreenRecording2024-10-27at11 38 40AM-ezgif com-video-to-gif-converter

We should add overflow: "hidden" to the global html and body css in AppRoute.tsx` like this:

  "@global html": {
    scrollPaddingTop: `calc(${theme.shape.navPaddingXs} + ${theme.spacing(2)})`,
    height: "100%",
    overflow: "hidden" // here
  },
  [theme.breakpoints.up("sm")]: {
    "@global html": {
      scrollPaddingTop: `calc(${theme.shape.navPaddingSmUp} + ${theme.spacing(
        2
      overflow: "hidden" // here
      )})`,
    },
  },
  "@global body": {
    height: "100%",
    overflow: "hidden" // here
  },

Right now that breaks the scroll in other places, so the main part of this ticket will be adding the overflow: "auto" to the specific components that should have it (below the nav bar) after making this change. For example the dashboard landing page no longer scrolls.

Make sure to check mobile and desktop.

@bakeiro
Copy link
Contributor

bakeiro commented Oct 28, 2024

I think this can be done in the Next.js config... it's what I did on the map, maybe makes sense to always use the same solution for the problem? (instead of multiple solutions for the same problem)

@bakeiro
Copy link
Contributor

bakeiro commented Oct 28, 2024

@nabramow
Copy link
Contributor Author

nabramow commented Oct 29, 2024

@nabramow

you can check here :) https://github.com/Couchers-org/couchers/blob/develop/app/web/pages/search.tsx#L34

Good point, I found the `variant: "full-width" after I wrote this. We should def see if there are other pages that should have this though still I think!

The messages one will be done in #5055.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.topic frontend 3.skill good first issue improvement Not really a feature but also not a bug
Projects
Status: No status
Development

No branches or pull requests

2 participants