[Tabs] New option to show scrollbar #22438
Conversation
|
@material-ui/core: parsed: +0.09% , gzip: +0.14% |
oliviertassinari
added
the
component: tabs
|
@LogyLeo Thanks for the pull request, I have a new commit coming to handle IE 11. |
There was a problem hiding this comment.
Awesome! I have applied the following changes:
- Use a boolean prop to trigger the behavior as we don't need 3 states
- Fix broken logic on IE 11, as the CSS approach support in the platform we target increases, we should be able to drop
ScrollbarSize. https://caniuse.com/mdn-css_properties_scrollbar-width_none - I have removed the new demo to keep it simple. I doubt it's something a lot of developers are looking for, hopefully a simple note in the markdown is enough to tip people. We will see.
Please review :)
|
@LogyLeo Could you fix the last standing issues with the CI? It will help, thanks! I believe the unit test fails, we might want to add a simple test case, but not sure it's very relevant. |
|
@oliviertassinari thanks a lot for your responsiveness on the subject! We don't need 3 states indeed; I kept the format used for scroll buttons, since it's a similar feature and may need to have "always on" and "desktop only" options just as well. But you're right, for now a boolean looks better! I'd like to fix the issues with CI, although I'm not familiar with the tool. The logs indicate that a change in class names causes the error. Where are CI tests written, and can I edit them? |
|
@LogyLeo Looks like we changed the name of the class names (scrollable -> scrollableX), maybe the tests can't find the relevant node? |
|
@oliviertassinari tests passed! :) Thanks for your support. |
@LogyLeo Likely this weekend as v5.0.0-alpha.9 |
|
@LogyLeo It's a great first pull request on Material-UI 👌🏻. Thank you for working on it! |
Added an option to show the scrollbar on Tabs component + updated docs accordingly
Closes #22432