mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.97k stars 32.28k forks source link

[Tabs] Update to match the specification #15324

Open eps1lon opened 5 years ago

eps1lon commented 5 years ago

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 https://github.com/mui-org/material-ui/pull/13980#issuecomment-482465309

https://material.io/design/components/tabs.html

eps1lon commented 5 years ago

@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?

oliviertassinari commented 5 years ago

@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.

olee commented 5 years ago

Our page is quite dynamic and there can be a varying amount of tabs and screen sizes. Making the tabs fullWidth ensures that when there are only a few or the screen is wide, they are filling out the available space. But if there are many tabs or the user is on mobile, it will properly provide scrolling behavior for the tabs so the user is able to access all panels. Choosing either one of these two options is a decision that a library should force upon users, if not really necessary.

On the other hand: Is there any good reason, to force users using only one of both options?

oliviertassinari commented 5 years ago

@olee Thank you for the details. In your case, I think that the fullWidth prop was abused. It's meant to be used on mobile, not on desktop.

oliviertassinari commented 5 years ago

@olee Did you found a reasonable workaround?

olee commented 5 years ago

No, I am right now still using the deprecated props and getting the warning. Also what do you mean with abusing the fullWidth prop?

oliviertassinari commented 5 years ago

The fullWidth prop targets mobile users that have no scrollbar. You seem to use it with scrollbar on desktop.

mbrookes commented 5 years ago

@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."

oliviertassinari commented 5 years ago

So it works with a tablet too?

mbrookes commented 5 years ago

It looks like desktop to me (icons top-right):

image

But I agree, it is best used for smaller screen widths.

The fullWidth prop targets mobile users that have no scrollbar.

That's mixing up two different things. The use of fullWidth has nothing to do with whether the device has a scrollbar.

To summarise the v2 spec, there are two primary variants: "Fixed" and "Scrollable".

Fixed tabs are of equal width (MUI default†), and can be aligned left (variant="standard"), center (centered prop) or right (not supported); or be full-width (variant="fullWidth").

Not enforced for longer labels.

Scrollable tabs (variant="scrollable") are the minimum width needed for the label, to minimise scrolling (MUI does not follow the spec).

  1. The API seems a bit inconsistent at the moment.
  2. We don't follow the spec for scrollable.
  3. There's no obvious way to responsively switch between variants.

(Incidentally, scrollButtons="off" looks broken on Mac. There is a permanent scrollbar overlaying the tabs): image

oliviertassinari commented 5 years ago
  1. The API seems a bit inconsistent at the moment.

In what sense?

  1. We don't follow the spec for scrollable.

What specific aspect aren't we following?

  1. There's no obvious way to responsively switch between variants.

Do we need to responsively switch between variants? Can the useMediaQuery() serves this purpose?

(Incidentally, scrollButtons="off" looks broken on Mac. There is a permanent scrollbar overlaying the tabs):

We are keeping track of the problem in #14706. It's a low hanging fruit.

mbrookes commented 5 years ago

In what sense?

Fixed tabs are of equal width (MUI default†), and can be aligned left (variant="standard"), center (centered prop) or right (not supported); or be full-width (variant="fullWidth").

Not enforced for longer labels.

What specific aspect aren't we following?

Scrollable tabs (variant="scrollable") are the minimum width needed for the label, to minimise scrolling (MUI does not follow the spec).

oliviertassinari commented 5 years ago

v4

It seems we haven't clean the Tabs component from the deprecated props. We still have the deprecatedPropType( usage and the corresponding logic. What should do about it? Should we revert or remove the old logic?

v5

What 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:

mbrookes commented 5 years ago

How about:

interface TabsProps {
  variant:? 'fixed' | 'scrollable'; // Fixed is fullwidth by default, justifyContent overrides it.
  justifyContent:? 'flex-start' | 'center' | 'flex-end';
  flexDirection:? 'row' | 'column';
}

variant enum eliminates the possibility of an invalid combination of fixed and scrollable props. justifyContent could possibly work for both fixed and scrollable, setting the default position of the items for scrollable when there are less items than would overflow the current display width.

oliviertassinari commented 5 years ago

variant enum eliminates the possibility of an invalid combination of fixed and scrollable props.

@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 (fullWidth on small screens) or to set the min-width style (fixed on large screens).

justifyContent could possibly work for both fixed and scrollable

I think that we should warn when justifyContent is used with scrollable. It doesn't work, at least, when I try it with the dev tools.

Regarding @olee use case, how do you support it with this API? I was proposing the same API as before <Tabs scrollable fullWidth />.

mbrookes commented 5 years ago

How does it work with case n°1 and n°2?

Case n°1:

<Tabs variant="fixed">, or since that's the default, just:

<Tabs>

Case n°2:

<Tabs justifyContent="center">.

The alternative to the variant prop is just to have the scrollable prop. No need for a fixed prop, since that's the default. This closes the door on future variants though (without future a breaking change to introduce a variant prop).

I think that we should warn when justifyContent is used with scrollable

That's the alternative, yes.

Regarding @olee use case, how do you support it with this API?

If the content isn't scrollable (doesn't overflow), fall back to fixed, with whatever option has been set for justifyContent.

oliviertassinari commented 5 years ago

case n°1, case n°2.

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';
}

No need for a fixed prop, since that's the default.

Why should fixed by the default state of the Tab? What do you think of this state as the default?

Capture d’écran 2019-04-21 à 18 05 43

<Tabs justifyContent="flex-start" scrollable={false} fullWidth={false} flexDirection="row" fixed={false} />
mbrookes commented 5 years ago

justifyContent:? null | 'flex-start' | 'center' | 'flex-end';

Right.

Why should fixed by the default state of the Tab?

Well it's either got to be fixed or scrollable, so fixed seems like the sensible default.

What do you think of this state as the default?

<Tabs justifyContent="flex-start" scrollable={false} fullWidth={false} flexDirection="row" fixed={false} />

Why are we back to having two opposing props? The variant is either fixed or scrollable, so the choice is either having a variant prop; or having a scrollable prop that overrides the the default (which in this case would be fixed).

What is the state of the Tabs if you have two props, and the both default to false? No tabs?

And what is the state if both are set to true? We introduced the variant prop to solve that problem, so why not use it here?

olee commented 5 years ago

Regarding @olee use case, how do you support it with this API? Well as I said above, we are using scrollable and fullWidth at the same time, but at the same time setting a minWidth on the tabs so that the text is not cut off and instead it starts to scroll on smaller screens. I think I can provide an example if necessary.

olee commented 5 years ago

Ok I played around a bit and noticed that it's also possible to use variant='scrollable' and a custom flex: 1 css property on the child Tab elements: https://codesandbox.io/s/r53v05413q

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.

oliviertassinari commented 5 years ago

Well it's either got to be fixed or scrollable

@mbrookes I would advocate for a hybrid solution as the default. In the screenshot I have shared: https://github.com/mui-org/material-ui/issues/15324#issuecomment-485263300, 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.

Why are we back to having two opposing props?

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.

mbrookes commented 5 years ago

" 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.

oliviertassinari commented 5 years ago

@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 deprecatedPropType usage as well as the logic behind it. 📦 savings.


For people using <Tabs fullWidth scrollable /> in v3, the solution is the following in v4:

const StyledTab = withStyles({
  root: {
    flex: 1,
  },
})(Tab);
mbrookes commented 5 years ago

What do you think of deferring this effort to v5?

Yup, we were discussing this API change in the context of v5 per your original API proposal: https://github.com/mui-org/material-ui/issues/15324#issuecomment-485245638

The best proposal we have found yet

Looks bang-on to me.

jacobweber commented 5 years ago

If possible, it would be great if you could allow the fullWidth style as long as there's room, but with scrollable turned on as well. A bunch of us have been hacking previous versions to do that.

olee commented 5 years ago

That's exactly what I was talking about - both options should be usable at the same time imho.

siriwatknp commented 3 years ago

Our page is quite dynamic and there can be a varying amount of tabs and screen sizes. Making the tabs fullWidth ensures that when there are only a few or the screen is wide, they are filling out the available space. But if there are many tabs or the user is on mobile, it will properly provide scrolling behavior for the tabs so the user is able to access all panels. Choosing either one of these two options is a decision that a library should force upon users, if not really necessary.

On the other hand: Is there any good reason, to force users using only one of both options?

Is this enough to achieve the expected behaviour? https://codesandbox.io/s/basictabs-material-demo-forked-lbvyc?file=/demo.tsx

olee commented 3 years ago

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. Perhaps it could be added as an example to the docs for other users as well.