-
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): Add MDCTab component #2421
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2421 +/- ##
==========================================
+ Coverage 98.86% 98.88% +0.01%
==========================================
Files 101 104 +3
Lines 4149 4210 +61
Branches 529 533 +4
==========================================
+ Hits 4102 4163 +61
Misses 47 47
Continue to review full report at Codecov.
|
@@ -56,6 +57,7 @@ autoInit.register('MDCLinearProgress', linearProgress.MDCLinearProgress); | |||
autoInit.register('MDCNotchedOutline', notchedOutline.MDCNotchedOutline); | |||
autoInit.register('MDCRadio', radio.MDCRadio); | |||
autoInit.register('MDCSnackbar', snackbar.MDCSnackbar); | |||
autoInit.register('MDCTab_', tab.MDCTab); |
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.
A working but imperfect solution to avoid naming collision.
packages/mdc-tab/index.js
Outdated
} | ||
|
||
/** @return {!MDCRipple} */ | ||
get ripple() { |
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.
do we need a get ripple
method on MDCTab?
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.
Not sure. I was following other components and exposing a getter for MDCRipple seems to be common practice.
But is it necessary? Probably not.
/** | ||
* The [de]activation animation affects color for text label and icon | ||
*/ | ||
.mdc-tab--animating-activate .mdc-tab__text-label, |
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 just have an mdc-tab--animating
class?
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.
The separate classes are because activation has a 100ms delay.
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.
Offline discussion: keep em both
packages/mdc-tab/package.json
Outdated
{ | ||
"name": "@material/tab", | ||
"description": "The Material Components for the web tab component", | ||
"version": "0.33.0", |
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.
this should be version 0.0.0 I think...then Lerna up the version number later.
I forget where the docs are, but somewhere there are docs saying how to create a new package.
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.
Dope, I'll check that out.
@@ -63,6 +63,7 @@ | |||
"slider": "/demos/slider.scss", | |||
"snackbar": "/demos/snackbar.scss", | |||
"switch": "/demos/switch.scss", | |||
"tab": "/demos/tab.scss", |
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.
Ask Andy if we still need these golden files. I think the webpack refactor is over...so maybe we can remove these files
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.
Confirmed with @acdvorak these are necessary for now.
Fixes #2376