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

[Tabs] Scroll by width of the first visible tab if only one tab is partially visible #32778

Merged
merged 3 commits into from Jun 10, 2022

Conversation

frankkluijtmans
Copy link
Contributor

@frankkluijtmans frankkluijtmans commented May 15, 2022

Currently, you're not able to scroll tabs when the container is narrow enough to have only one tab (partially) visible. This is my proposed solution, which would scroll by the width of the first visible tab in such a case.

Fixes #32748

@mui-bot
Copy link

mui-bot commented May 15, 2022

Details of bundle changes

Generated by 🚫 dangerJS against 57bc172

@oliviertassinari oliviertassinari added PR: needs test The pull request can't be merged component: tabs This is the name of the generic UI component, not the React module! and removed PR: needs test The pull request can't be merged labels May 15, 2022
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Nice! Based on https://codesandbox.io/s/scrollabletabsbuttonforce-material-demo-forked-bzqr8l?file=/package.json looks like the solution works. Let's add test to ensure we won't create another regression in the future.

@mnajdova mnajdova added the PR: needs test The pull request can't be merged label May 18, 2022
@frankkluijtmans
Copy link
Contributor Author

@mnajdova Thanks for the review. The test I added in Tabs.test.js should already cover this case 😄

packages/mui-material/src/Tabs/Tabs.js Outdated Show resolved Hide resolved
@frankkluijtmans frankkluijtmans force-pushed the fix/tabs-horizontal-scroll branch 5 times, most recently from 4aba1c4 to 91bfa8b Compare June 2, 2022 20:19
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Much better. Left few comments for simplifying the logic, and one thing that should be double checked. Good job with the tests!

packages/mui-material/src/Tabs/Tabs.js Outdated Show resolved Hide resolved
packages/mui-material/src/Tabs/Tabs.js Outdated Show resolved Hide resolved
packages/mui-material/src/Tabs/Tabs.js Outdated Show resolved Hide resolved
packages/mui-material/src/Tabs/Tabs.js Outdated Show resolved Hide resolved
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

It's a great first pull request on MUI 👌 Thank you for working on it!

@mnajdova mnajdova merged commit 66f7a9d into mui:master Jun 10, 2022
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work and removed PR: needs test The pull request can't be merged labels Jun 11, 2022
@frankkluijtmans frankkluijtmans deleted the fix/tabs-horizontal-scroll branch June 14, 2022 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: tabs This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tabs] Tab container won't scroll when only one tab is (partially) visible
4 participants