feat(tabs): Add tab indicator inside tab #2565
Conversation
Codecov Report
@@ Coverage Diff @@
## feat/tabs/tabs #2565 +/- ##
==================================================
+ Coverage 98.78% 98.79% +<.01%
==================================================
Files 103 103
Lines 4277 4303 +26
Branches 535 536 +1
==================================================
+ Hits 4225 4251 +26
Misses 52 52
Continue to review full report at Codecov.
|
| @@ -27,6 +27,12 @@ | |||
| } | |||
| } | |||
|
|
|||
| @mixin mdc-tab-indicator-not-upgraded-active_ { | |||
There was a problem hiding this comment.
The mdc-tab-indicator-active_ mixin is only called within the mdc-tab-indicator.scss file right? Why does the mdc-tab-indicator-not-upgraded-active_ mixin called within the mdc-tab.scss file?
There was a problem hiding this comment.
mdc-tab-indicator-not-upgraded-active makes the active tab indicator visible until the tab indicator is upgraded and initialized. I'll make it non-private and rename it while also removing mdc-tab-indicator-active as it serves no real purpose.
| @@ -84,16 +96,20 @@ class MDCTabFoundation extends MDCFoundation { | |||
|
|
|||
| /** | |||
| * Activates the Tab | |||
| * @param {!ClientRect=} previousIndicatorClientRect | |||
There was a problem hiding this comment.
I forget my closure syntax...but can previousIndicatorClientRect be undefined?
There was a problem hiding this comment.
It's optional. The = makes it optional (undefined) where as the ? makes it nullable. We want it to be optional in this case.
| this.adapter_.registerEventHandler('transitionend', this.handleTransitionEnd_); | ||
| this.adapter_.addClass(cssClasses.ANIMATING_ACTIVATE); | ||
| this.adapter_.addClass(cssClasses.ACTIVE); | ||
| this.adapter_.setAttr(strings.ARIA_SELECTED, 'true'); | ||
| this.adapter_.setAttr(strings.TABINDEX, '0'); |
There was a problem hiding this comment.
Why do you need to add tab-index to the tab when it is activated? Shouldnt tabs always have tab-index?
There was a problem hiding this comment.
The ARIA tab doc says that only the active tab should be accessible via keyboard focus. There's a good explanation here: https://www.w3.org/TR/wai-aria-practices-1.1/#tabpanel
From the ARIA Tab Example:
| Key | Function |
|---|---|
| Tab | When focus moves into the tab list, places focus on the active tab element. When the tab list contains the focus, moves focus to the next element in the tab sequence, which is the tabpanel element. |
| Right Arrow | Moves focus to the next tab.If focus is on the last tab, moves focus to the first tab. Activates the newly focused tab. |
| Left Arrow | Moves focus to the previous tab.If focus is on the first tab, moves focus to the last tab. Activates the newly focused tab. |
| Home | Moves focus to the first tab and activates it. |
| End | Moves focus to the last tab and activates it. |
| this.adapter_.registerEventHandler('transitionend', this.handleTransitionEnd_); | ||
| this.adapter_.addClass(cssClasses.ANIMATING_DEACTIVATE); | ||
| this.adapter_.removeClass(cssClasses.ACTIVE); | ||
| this.adapter_.setAttr(strings.ARIA_SELECTED, 'false'); | ||
| this.adapter_.setAttr(strings.TABINDEX, '-1'); |
There was a problem hiding this comment.
-1 for deactivate? See question above.
There was a problem hiding this comment.
See above comment
packages/mdc-tab/index.js
Outdated
| * computeIndicatorClientRect: function(): !ClientRect, | ||
| * }} | ||
| */ | ||
| getIndicatorAdapterMethods_() { |
There was a problem hiding this comment.
I dont think you need to make this a separate method. Just move these three methods into the adapter class above.
packages/mdc-tab/mdc-tab.scss
Outdated
| @include mdc-tab-indicator-not-upgraded-active_; | ||
| } | ||
|
|
||
| .mdc-tab:not(.mdc-tab--stacked) .mdc-tab__icon + .mdc-tab__text-label { |
There was a problem hiding this comment.
this CSS selector is so complicated! whats it doing?
There was a problem hiding this comment.
It's setting the left padding of the text label only when it's preceded by an icon. There should only be a left padding when the tab is horizontal (the default state, aka "not stacked").
There was a problem hiding this comment.
I simplified this.
packages/mdc-tab-indicator/README.md
Outdated
| #### Active Indicator | ||
|
|
||
| ```html | ||
| <span class="mdc-tab-indicator mdc-tab-indicator--active"> |
There was a problem hiding this comment.
actually i think this can just be a CSS class, you dont need the whole HTML structure
WIP fixed bg coloring of icons fix(tab-indicator): Use absolute positioning (#2547) WIP start of tab scroller WIP fixed transition duration WIP progress on scroller WIP added demos back chore(tabs): Removed tab scroller feat(tabs): Add tab indicator inside tab (#2565) feat(tab-scroller): Add tab scroller (#2577) Merge master into feat/tabs/tabs (#3096) feat(tab): Update tab color and typography (#3108) docs(tabs): Update metadata and synopses (#3117) feat(tab): Add MDCTabDimensions computation (#3122) feat(tab): Emit selection and activation events (#3139) docs(tabs): Update new READMEs to match standard (#3142) feat(tab): Give focus to tab when activated (#3164) feat(tab): Add mixin for parent positioning; Use mixin in tab scroller (#3179) fix(tabs): Suppress area occupied by scrollbar on platforms that show it (#3149) fix(tab): Remove extraneous padding from the stacked text label in LTR (#3193) feat(tabs): Add missing docs and create helper util API (#3194) Merge master into feat/tabs/tabs (#3227) feat(tab): Update layout; Add fixed-width mixin; Add min-width class (#3220) fix(tab-scroller): Fix incorrect animation stopping scroll value in RTL (#3237) feat(tab-scroller): Add scroll content width method for use in tab bar (#3222) feat(tab): Remove activation event emitting (#3242) feat(tab-bar): Add tab bar (#3229)
Closes #2550