-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat(tab): Implement a ripple color mixin for Tab #4421
Conversation
All 624 screenshot tests passed for commit d0db5c3 vs. |
packages/mdc-tab/_mixins.scss
Outdated
@@ -36,6 +37,17 @@ | |||
} | |||
} | |||
|
|||
@mixin mdc-tab-ripple-color($color) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we call this mdc-tab-states-color given that it's invoking mdc-states?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea.
Just an aside: the mixed naming between "ripple" and "states" keeps tripping me up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey wait we should probably update the README right?
Yeah sorry about that. I will update the docs. |
All 624 screenshot tests passed for commit 40c4a67 vs. |
Added docs for the mixins. |
} | ||
} | ||
|
||
@mixin mdc-tab-ink-color($color) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is here... should this also set mdc-tab-icon-color so that everything is set together? (I forget why text and icon color were originally separate, maybe @patrickrodee can weigh in on this.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had separate mixins for text and icon in other components when I added these mixins. Setting icon
color in the ink
mixin makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, my definition of ink was outdated. Done.
All 624 screenshot tests passed for commit 504dbe9 vs. |
All 624 screenshot tests passed for commit df208c1 vs. |
All 624 screenshot tests passed for commit 5276f45 vs. |
All 624 screenshot tests passed for commit ad53d28 vs. |
No description provided.