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

[website] Hide scrollbars of hero containers #29474

Merged
merged 4 commits into from
Feb 2, 2022

Conversation

theiliad
Copy link
Contributor

@theiliad theiliad commented Nov 2, 2021

for some reason there's a scrollbar showing up inside of this animated perspective box. This PR should remove that

image

https://mui.com/templates/

@mui-pr-bot
Copy link

mui-pr-bot commented Nov 2, 2021

No bundle size changes

Generated by 🚫 dangerJS against 360220b

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 2, 2021

Looks like a regression from #29141 to fix #29119. I find it weird that we can scroll the right side on https://mui.com/.

@oliviertassinari oliviertassinari added the website Pages that are not documentation-related, marketing-focused. label Nov 2, 2021
@theiliad
Copy link
Contributor Author

theiliad commented Nov 2, 2021

Looks like a regression from #29141 to fix #29119. I find it weird that we can scroll the right side on https://mui.com/.

oh I see. Yeah I didn't have enough time to find where it's coming from, so just added the overflow:hidden

I don't know why you'd want the templates hero to be scrollable though 🤔 It already "scrolls" automatically through the animation... Homepage makes more sense

@oliviertassinari
Copy link
Member

for some reason there's a scrollbar showing up inside of this animated perspective box.

I can reproduce the same issue on https://deploy-preview-29474--material-ui.netlify.app/design-kits/.


On https://deploy-preview-29474--material-ui.netlify.app/x/, there are 3 different scroll containers, it's weird

Screenshot 2021-11-02 at 20 58 19

@hbjORbj Do you want to lead the fix since you started in #29141?

Speaking of scrollbar for macOS maintainers, I would recommend to always be using:

Screenshot 2021-11-02 at 20 59 10

as it matches what Windows end-users (what the large majority of people use) experience.

@theiliad
Copy link
Contributor Author

theiliad commented Nov 2, 2021

I can reproduce the same issue on https://deploy-preview-29474--material-ui.netlify.app/design-kits/.

Yes, I'm not super familiar with the various sections of the site and the goals laid out for each. What I would've done would be to make them ALL non-scrollable & ONLY update the 1 or 2 pages that actually need to be scrollable

@theiliad
Copy link
Contributor Author

theiliad commented Nov 2, 2021

as it matches what Windows end-users (what the large majority of people use) experience.

with that option selected I still see the extra scrollbar

@hbjORbj
Copy link
Member

hbjORbj commented Nov 3, 2021

@hbjORbj Do you want to lead the fix since you started in #29141?

@oliviertassinari Yep, I will make a fix for this after I address some other priorities!

@hbjORbj hbjORbj changed the title fix(templates-hero): perspective box scroll overflow [website] Make hero containers not scrollable Dec 27, 2021
@hbjORbj hbjORbj changed the title [website] Make hero containers not scrollable [website] Hide scrollbars of hero containers Dec 27, 2021
@hbjORbj hbjORbj force-pushed the fix-templates-hero-overflow branch from 1332da7 to a89eb56 Compare December 27, 2021 11:20
@hbjORbj
Copy link
Member

hbjORbj commented Dec 27, 2021

@oliviertassinari

I've summarised the final results below:

  1. The scrollbars from all hero containers (/, /x. /design-kits, /templates) are removed.
  2. The hero in main home page (/) is scrollable (I think this makes sense because some users want to see the components).
  3. The hero in design-kits page (/design-kits) is NOT scrollable (I think this makes sense because it has scroll animation itself).
  4. The hero in templates page (/templates) is NOT scrollable (I think this makes sense because it has scroll animation itself).

Preview: https://deploy-preview-29474--material-ui.netlify.app

@danilo-leal
Copy link
Contributor

@hbjORbj Maybe /core and /x need to be scrollable as well?

@hbjORbj
Copy link
Member

hbjORbj commented Dec 27, 2021

@hbjORbj Maybe /core and /x need to be scrollable as well?

@danilo-leal Sure, I made /core's hero scrollable as well so that it is consistent with that of /.

Currently, the heroes on / and /core are vertically scrollable but not horizontally.
The hero on /x is horizontally scrollable but not vertically.

  1. Should I add vertical scrollability to /x as well?
  2. Do you think not having horizontal scrollability on / and /core is fine?

@danilo-leal
Copy link
Contributor

I think I'd standardize it, have /, /core and /x scrollable on both axis!

@mui-bot
Copy link

mui-bot commented Feb 1, 2022

No bundle size changes

Generated by 🚫 dangerJS against 117c002

@danilo-leal
Copy link
Contributor

I'll merge this one since it has been open for quite a while and /, /core and/x heroes are now scrollable.

Must admit though that I kinda feel the same as Olivier - these heroes being scrollable is kinda weird, it just doesn't seem very polished to me. I think we can iterate in future PRs for ways to make this interaction more elegant.

@danilo-leal danilo-leal merged commit 68b8464 into mui:master Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
website Pages that are not documentation-related, marketing-focused.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants