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] Improve color management #24286

Closed
skondrashov opened this issue Jan 6, 2021 · 7 comments · Fixed by #25063
Closed

[Tabs] Improve color management #24286

skondrashov opened this issue Jan 6, 2021 · 7 comments · Fixed by #25063
Labels
breaking change component: tabs This is the name of the generic UI component, not the React module! new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Milestone

Comments

@skondrashov
Copy link

skondrashov commented Jan 6, 2021

Tabs by default receive the MuiTab-textColorInherit class, which for some reason sets their opacity to .7. Honestly I don't know what I'm missing, it's not in the docs but nobody on the internet seems to have complained about this. I can't imagine why this was included except to discourage developers from using inherit, in which case shouldn't it just be removed, or at least an opt-in behavior instead of a default? It's been in the code for 3 years but I can't wrap my mind around the decision here. At risk of stating the obvious, I expected that Tabs using inherited colors would be colored with the colors they inherited, and not mixed with 30% of the colors behind them. I'm forced to add an opacity rule to my theme for tabs if I want the obvious behavior, and figure out on my own why the colors are wrong given that it doesn't appear to be documented.

What you need to put in your theme overrides to fix this in case other people find this:

const theme = createMuiTheme({
  overrides: {
    MuiTab: {
      textColorInherit: {
        opacity: 1,
      },
    },
  },
});
@skondrashov skondrashov added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jan 6, 2021
@oliviertassinari oliviertassinari added component: tabs This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation new feature New feature or request and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer docs Improvements or additions to the documentation labels Jan 7, 2021
@oliviertassinari oliviertassinari added this to the v5 milestone Jan 7, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 7, 2021

@tkondrashov Do you have a visual example of what you are trying to achieve?

The opacity is used to distinguish between selected and unselected tab. I think that we could do two things:

  1. Maybe primary the default color. It's what you will find in https://material.io/components/tabs or in https://console.cloud.google.com/, or in https://support.google.com/chrome/. It's also related to [Tabs] Update to match the specification #15324.
  2. Add a section in the documentation about the different colors https://codesandbox.io/s/basictabs-material-demo-forked-db6i1?file=/demo.tsx.

@oliviertassinari oliviertassinari changed the title [Tab] tabs are semitransparent by default, not documented [Tabs] Improve color management Jan 7, 2021
@oliviertassinari oliviertassinari added the ready to take Help wanted. Guidance available. There is a high chance the change will be accepted label Jan 7, 2021
@skondrashov
Copy link
Author

skondrashov commented Jan 8, 2021

@oliviertassinari There were two concerns in my particular case:

  1. I am following a style guide, and we want black on white text for these tabs, but the text and background match with regular content text/background, so inheriting seems to be a fine choice.
  2. My code has the tabs directly above a table with a white background, where clicking a tab switches the content of the table:
    image
    Especially without a border between the tabs and the table (we have one, but I omitted it to illustrate) the very slight difference between the table and the tabs due to the white taking on some of the color of the greyish page background looks just slightly annoying.

In the examples in the docs, some of them avoid this issue (intentionally or not) by giving the Tabs a transparent background and having them on an AppBar with a solid background, but as it isn't a requirement to use Tabs with an AppBar it feels like a workaround even heavier than the opacity adjustment in the theme.

What I don't really understand about the need to distinguish tabs is that's what the ink bar is already for, isn't it?
I also don't immediately see why that would be an issue only if you inherit colors, but not if you specify primary/secondary - wouldn't you still have the same problem of needing to distinguish tabs regardless of which palette you specify? I haven't tried to use a palette so I could just not know why it works differently.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 8, 2021

@tkondrashov Thanks for the visual illustration, perfect, it's a confirmation of #24286 (comment).

In your design, the text of the selected tab is #191919 and the other tabs are lighter: #4D4E4E. The opacity aims to reproduce the same outcome.
It also looks like you are no looking for using <Tabs textColor="inherit" indicatorColor="secondary" /> (the default) but <Tabs textColor="primary" indicatorColor="primary" /> which I have proposed to make the default because closer to what developers need.

Do you want to work on it?

@skondrashov
Copy link
Author

@oliviertassinari That screenshot was a demonstration of the problems I encountered without the opacity override, so ideally all of them would be #191919 for my case. But yes, if you think that defaulting to primary is a good idea I would find that more intuitive, and I'd be happy to work on it! I would also think it's a good idea to note in the docs that "inherit" will change the opacity.

@oliviertassinari
Copy link
Member

@tkondrashov ah, right, thanks for the clarification. Contributions welcome!

@Dripcoding
Copy link
Contributor

@oliviertassinari I'd like to work on this issue since it's been more than a month.

@oliviertassinari
Copy link
Member

@Dripcoding You can go ahead 🟢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: tabs This is the name of the generic UI component, not the React module! new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
3 participants