feat/tabs/sass color mixins

[moog16]

fixes: #1152

[codecov-io]

Codecov Report

Merging #1801 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1801   +/-   ##
=======================================
  Coverage   99.44%   99.44%           
=======================================
  Files          84       84           
  Lines        3626     3626           
  Branches      456      456           
=======================================
  Hits         3606     3606           
  Misses         20       20

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd5ea7b...c0bf0de. Read the comment docs.

[lynnmercier]

Good start, left some comments.

I about to head out on holidays, so take my feedback into consideration, but get the rest of the team to help you while Im out.

[lynnmercier]

This should not be in this PR

[lynnmercier]

I think you should either
A) Send out a "pre-PR" which refactors the Tabs README to fit wit our REAMDE standards:
https://github.com/material-components/material-components-web/blob/master/docs/code/readme_standards.md
B) OR only update the mdc-tabs/README.md around the Sass mixins you add in this PR

[lynnmercier]

so if you change the color of mdc-tab-bar__indicator...you also need to change the opacity of mdc-tab?

[moog16]

you're right confusing. I think it is better to go with something like:

@mixin mdc-tab-indicator-ink-color($color, $opacity);
@mixin mdc-tab-label-ink-color($color, $opacity);
@mixin mdc-tab-icon-ink-color($color, $opacity);

[moog16]

Nevermind...we don't need $opacity. Developers and clients can just go like:

@include mdc-tab-indicator-ink-color(rgba(0,0,0, 0.9))

[lynnmercier]

I dont think you want to call mdc-tab-icon-ink-color from within mdc-tab-label-ink-color. This makes it impossible to customize the ink color of the icon separate from the ink color of the label.

Instead, have:

• mdc-tab-label-ink-color as a mixin which only sets the label ink color
• mdc-tab-icon-ink-color as a mixin which only sets the icon ink color
• mdc-tab-ink-color which sets all ink color within the tab to the same color (right now thats label and icon, but someday we might add another subelement to tab, which would qualify as "ink")

[moog16]

This way actually works where you can customize the ink and the icon color with these two mixins. But I agree it isn't clear. I'll go with the three mixins.

[lynnmercier]

I go back and forth on the best way to support states within an element. In this case its how to support customization of both the inactive and active state of a tab.

In mdc-button, we did not want to allow customization of the disabled button, so we added a :not(:disabled) to our public mixins.

However, in this mdc-tab case, I think we want to support customization of active and inactive tabs separately.

I propose this:

• one public sass mixins mdc-tab-label-ink-color
• drop mdc-tab-label-activated-ink-color
• change mdc-tab-bar.scss to have:

.mdc-tab {
  @include mdc-tab-label-ink-color(text-secondary-on-light);
}
.mdc-tab--active {
  @include mdc-tab-label-ink-color(text-primary-on-light);
}

Then clients should override the the active inactive state like this

.foo-tab:not(.mdc-tab--active) {
  @include mdc-tab-label-ink-color(pink);
}
.foo-tab.mdc-tab--active {
  @include mdc-tab-label-ink-color(purple);
}

I think it is more important that clients keep their active and inactive states in sync, then it is for clients to keep their subcomponents in sync. For example this makes a lot of sense:

.foo-tab:not(.mdc-tab--active) {
  @include mdc-tab-label-ink-color(pink);
  @include mdc-tab-icon-ink-color(pink);
}
.foo-tab.mdc-tab--active {
  @include mdc-tab-label-ink-color(purple);
  @include mdc-tab-icon-ink-color(purple);
}

But this is harder to read

.foo-tab {
  @include mdc-tab-label-ink-color(pink /*inactive color*/, purple /*active color*/);
  @include mdc-tab-icon-ink-color(pink /*inactive color*/, purple /*active color*/);
}

[moog16]

If we go that route won't have mixins on the parent class, which I thought was what we wanted. Also the label-ink-color mixin would be a proxy to mdc-theme-prop:

@mixin mdc-tab-label-ink-color($color) {
  @include mdc-theme-prop(color, $color);
}

The icon would also just be:

@mixin mdc-tab-icon-ink-color($color) {
  @include mdc-theme-prop(color, $color);
}

What is the main purpose for these scss mixins? Is it to allow clients to customize the components quicker? To make our styles DRY'er? In either case if our scss mixins for other components are designed to live on the parent class, we should try to be consistent here as well. Clients and external developers can easily see the mixins we've created and know where to put them for quick configuration.

I'll probably pose the question on theming since you are on vacayy ;)

[lynnmercier]

actually, don't bother with indicator height and width right now. just focus on the color mixins for now

[moog16]

Ok will remove from PR

[moog16]

Closing as I am addressing changes in #1851