-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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] Update to match the specification #15324
Comments
@olee Could you elaborate why this was a breaking change? I would hope this only added a deprecation warning. Could you share your use case so that we can evaluate how this might be supported in the future when we remove these props? |
@olee What is your use case for enabling the full width and the scrollable behaviors at the same time. What's the expected tab width output? I fail to wrap by head about what would be an intuitive output. |
Our page is quite dynamic and there can be a varying amount of tabs and screen sizes. On the other hand: Is there any good reason, to force users using only one of both options? |
@olee Thank you for the details. In your case, I think that the |
@olee Did you found a reasonable workaround? |
No, I am right now still using the deprecated props and getting the warning. |
The fullWidth prop targets mobile users that have no scrollbar. You seem to use it with scrollbar on desktop. |
@oliviertassinari Is this not the same as: https://material.io/design/components/tabs.html#fixed-tabs ("Placement")? "On wider layouts, fixed tabs increase in width, as their tab width is determined by the width of the screen." |
So it works with a tablet too? |
In what sense?
What specific aspect aren't we following?
Do we need to responsively switch between variants? Can the
We are keeping track of the problem in #14706. It's a low hanging fruit. |
Fixed tabs are of equal width (MUI default†), and can be aligned left ( † Not enforced for longer labels.
Scrollable tabs ( |
v4It seems we haven't clean the Tabs component from the deprecated props. We still have the v5What do you think of this API? interface TabsProps {
scrollable:? boolean;
fullWidth:? boolean;
justifyContent:? 'flex-start' | 'center' | 'flex-end';
flexDirection:? 'row' | 'column';
fixed:? boolean; // sets a higher minimum width and a new maximum width
} With this API, we can reproduce the cases described by the specification, while being generic: <Tabs fullWidth /> <Tabs justifyContent="center" fixed /> <Tabs justifyContent="flex-end" fixed /> <Tabs scrollable />
<Tabs flexDirection="column" /> |
How about: interface TabsProps {
variant:? 'fixed' | 'scrollable'; // Fixed is fullwidth by default, justifyContent overrides it.
justifyContent:? 'flex-start' | 'center' | 'flex-end';
flexDirection:? 'row' | 'column';
}
|
@mbrookes How does it work with case n°1 and n°2? We need to either set the flex-grow: 1, flex-shrink: 1 style (
I think that we should warn when Regarding @olee use case, how do you support it with this API? I was proposing the same API as before |
Case n°1:
Case n°2:
The alternative to the
That's the alternative, yes.
If the content isn't scrollable (doesn't overflow), fall back to fixed, with whatever option has been set for |
If I follow you correctly, we need a null default prop for justifyContent, so: interface TabsProps {
variant:? 'fixed' | 'scrollable'; // Fixed is fullwidth by default, justifyContent overrides it.
justifyContent:? null | 'flex-start' | 'center' | 'flex-end';
flexDirection:? 'row' | 'column';
}
Why should <Tabs justifyContent="flex-start" scrollable={false} fullWidth={false} flexDirection="row" fixed={false} /> |
Right.
Well it's either got to be fixed or scrollable, so fixed seems like the sensible default.
Why are we back to having two opposing props? The variant is either What is the state of the Tabs if you have two props, and the both default to And what is the state if both are set to |
|
Ok I played around a bit and noticed that it's also possible to use const styles = {
tabRoot2: {
minWidth: 120,
flex: 1
}
} <Tabs value={0} variant="scrollable" scrollButtons="on">
<Tab label="Item One" classes={{ root: classes.tabRoot2 }} />
<Tab label="Item Two" classes={{ root: classes.tabRoot2 }} />
<Tab label="Item Three" classes={{ root: classes.tabRoot2 }} />
</Tabs> This would resolve the issue, however I still think that the variant change was kinda unnecessary. |
@mbrookes I would advocate for a hybrid solution as the default. In the screenshot I have shared: #15324 (comment), scrollable is off, fixed is off, fullWidth is off, flexDirection is column, justify-content is flex-start. It has the advantage of being easier to override (fewer styles) and closer to the other UI libraries. Then, people can tweak the props to match their need. You can think of this as a scrollable variant without the reserved space for the scroll <, > buttons.
I was reasoning in terms of CSS we would apply. The API I have proposed is close the underlying style rules we would apply but allows invalid combination. I'm not against your proposal. |
" In the screenshot I have shared: #15324 (comment), scrollable is off, fixed is off, [...]" There are two variants: "fixed", and "scrollable". If it isn't scrollable, it must be fixed. |
@mbrookes I have found the issue that summarizes my problem: #12358. What do you think of deferring this effort to v5? It will be a breaking change. I don't think that we have the time for v4. The best proposal we have found yet: interface TabsProps {
variant:? 'fixed' | 'scrollable';
// variant fixed is fullWidth by default, justifyContent overrides it. It doesn't make sense when variant is scrollable.
justifyContent:? null | 'flex-start' | 'center' | 'flex-end';
// column is used for vertical tabs #8662
flexDirection:? 'row' | 'column';
// v3 'auto' is 'desktop' in this proposal. #12358
scrollButtons?: 'auto' | 'on' | 'off' | 'desktop',
}
Tabs.defaultProps = {
variant: 'scrollable',
flexDirection: 'row',
scrollButtons: 'auto',
justifyContent: null,
}; If we agree with this direction, we can remove the For people using const StyledTab = withStyles({
root: {
flex: 1,
},
})(Tab); |
If possible, it would be great if you could allow the |
That's exactly what I was talking about - both options should be usable at the same time imho. |
Is this enough to achieve the expected behaviour? |
This looks quite good I guess - it's only a bit sad that previously such behavior was achievable just with the default options and now requires custom styles, however I could live with this. |
Hi, I have to disagree with this change.
We are using scrollable and fullWidth together just fine, it only requires adding a minWidth to the Tab button components but otherwise it works just fine.
This is a breaking change for our layout - could this be reverted / are there other solutions to keep fullWidth and scrollable available at the same time?
Originally posted by @olee in #13980 (comment)
https://material.io/design/components/tabs.html
The text was updated successfully, but these errors were encountered: