feat(tab): Add tab indicator #2461

patrickrodee · 2018-03-26T20:58:04Z

acdvorak · 2018-03-30T22:17:11Z

demos/tab-indicator.html

+        var demoItems = document.querySelectorAll('.demo-item');
+        var activeIndicator;
+        for (var i = 0; i < demoItems.length; i++) {
+          (function(i) {


Nit: Can you create a named function for this? An IIFE feels a little convoluted here.

acdvorak · 2018-03-30T22:18:57Z

demos/tab-indicator.html

+            var demoIndicator = demoItem.querySelector('.demo-indicator');
+            var indicator = new mdc.tabIndicator.MDCTabIndicator(demoIndicator);
+            var demoId = 'demo' + i;
+            window[demoId] = indicator;


Nit: Can we use a locally-scoped variable inside demoReady(function() { ...?

acdvorak · 2018-03-30T22:19:17Z

demos/tab-indicator.html

+            var demoRadio = demoItem.querySelector('input[type="radio"]');
+            var activeName = 'active_' + demoRadio.name;
+            if (demoIndicator.classList.contains('mdc-tab-indicator--active')) {
+              window[activeName] = indicator;


acdvorak · 2018-03-30T22:30:49Z

demos/tab-indicator.html

+      <section class="hero">
+        <div class="demo-inner">
+          <div class="demo-item">
+            <label class="demo-label"><input type="radio" name="demo1" checked>&nbsp;Active</label>


I'm not sure I understand this demo page:

For the custom icons, maybe you could have separate tab bars for each icon, and choose an icon color that's easier to see against the text?

And why do we need native radio buttons?

I used native radios because this is just the tab indicator demo, not the tab with tab indicator demo. The idea of the demo page is to show how the indicator can be used across multiple items.

I guess I should add some information about that.

lynnmercier

Looks really nice over all

lynnmercier · 2018-03-30T22:28:52Z

packages/mdc-tab-indicator/_mixins.scss

+@import "@material/theme/mixins";
+
+@mixin mdc-tab-indicator-active {
+  .mdc-tab-indicator {


dont put the "Block" CSS classname in the selector

lynnmercier · 2018-03-30T22:29:51Z

packages/mdc-tab-indicator/_mixins.scss

+    @include mdc-tab-indicator-bar-height_($height);
+  }
+
+  .mdc-tab-indicator--icon {


I guess this makes sense...you could have a separate mixin for just customizing the height of the icon variant...like mdc-tab-indicator-icon-height?

lynnmercier · 2018-03-30T22:31:54Z

packages/mdc-tab-indicator/bar-foundation.js

+import MDCTabIndicatorFoundation from './foundation';
+
+/**
+ * @final


I think you need an @extends in here somewhere

lynnmercier · 2018-03-30T22:39:50Z

packages/mdc-tab-indicator/bar-foundation.js

+    const xPosition = previousClientRect.left - currentClientRect.left;
+    this.adapter_.setStyleProperty('transform', `translateX(${xPosition}px) scaleX(${widthDelta})`);
+
+    // Force repaint


A getter should not cause any side effects. So putting "force repaint" above a call to getClientRect doesn't make sense

Will rename to computeBoundingRect to follow other naming schemes and disambiguate the side effects 👍

lynnmercier · 2018-03-30T22:42:32Z

packages/mdc-tab-indicator/bar-foundation.js

+/**
+ * @final
+ */
+class MDCTabIndicatorBarFoundation extends MDCTabIndicatorFoundation {


BarFoundation....or how about MDCSlidingTabIndicatorFoundation?

lynnmercier · 2018-03-30T22:44:10Z

packages/mdc-tab-indicator/foundation.js

+   */
+  handleTransitionEnd() {
+    this.adapter_.deregisterEventHandler('transitionend', this.handleTransitionEnd_);
+    this.adapter_.removeClass(MDCTabIndicatorFoundation.cssClasses.ANIMATING_ICON_ACTIVATE);


these icon thingys, should they be in MDCTabIndicatorIconFoundation?

lynnmercier · 2018-03-30T22:45:57Z

packages/mdc-tab-indicator/icon-foundation.js

+/**
+ * @final
+ */
+class MDCTabIndicatorIconFoundation extends MDCTabIndicatorFoundation {


I thought maybe I would see some logic around delaying the fade in and fade out. But I don't see anything in this file like that.

I wonder if it should be

activate() { super.activate(); this.adapter_.addClass(MDCTabIndicatorFoundation.cssClasses.ANIMATING_ICON_ACTIVATE); }

The delayed fade-in is in the CSS transition-end property.

The reason I avoided making the activate and deactivate concrete methods and instead made them abstract was to provide more granular control over when the handlers should be added or when to make a the indicator active.

That being said, I can make them concrete methods.

lynnmercier · 2018-03-30T22:56:15Z

demos/tab-indicator.html

@@ -0,0 +1,113 @@
+<!DOCTYPE html>


This demo page needs some restructure

don't put so much in the "hero" section. You can just put two tabs there showing hte default behavior and call it done

Add some sections, with titles, below the hero section. I'm thinking: bar x sliding indicator, icon x fading indicator, bar x fading indicator, icon x sliding indicator (same icon on each tab)

Also add sections for the customization: color, height, and corner radius

…nts/material-components-web into feat/tabs/tab-indicator

lynnmercier · 2018-04-05T23:28:08Z

demos/tab-indicator.html

+  See the License for the specific language governing permissions and
+  limitations under the License
+-->
+<html>


Add more cases where the tabs are different widths. And remove the fading bar demo.

lynnmercier · 2018-04-05T23:29:29Z

packages/mdc-tab-indicator/README.md

+
+#### Fading Icon Indicator
+
+We recommend you load [Material Icons](https://material.io/icons/) from Google Fonts. However, users are free to use whatever icons they like.


You can use Material Icons from Google Fonts within your Fading Icon Indicator, or you can use your own icons

lynnmercier · 2018-04-05T23:29:48Z

packages/mdc-tab-indicator/README.md

+</span>
+```
+
+#### Fading Bar Indicator


drop this section, since we dropped from demos

lynnmercier · 2018-04-05T23:32:17Z

packages/mdc-tab-indicator/_mixins.scss

+
+@import "@material/theme/mixins";
+
+@mixin mdc-tab-indicator-active {


Make this private-ish with an underscore, and sort to bottom of file

lynnmercier · 2018-04-05T23:33:12Z

packages/mdc-tab-indicator/_mixins.scss

+  opacity: 1;
+}
+
+@mixin mdc-tab-indicator-color($color) {


this mixin will probably end up outputting more CSS than the client needs. I would recommend we only support mdc-tab-indicator-bar-color and mdc-tab-indicator-icon-color.

lynnmercier · 2018-04-05T23:33:26Z

packages/mdc-tab-indicator/_mixins.scss

+  @include mdc-tab-indicator-icon-color($color);
+}
+
+@mixin mdc-tab-indicator-height($height) {


this mixin will probably end up outputting more CSS than the client needs. I would recommend we only support mdc-tab-indicator-bar-height and mdc-tab-indicator-icon-height.

lynnmercier · 2018-04-05T23:39:48Z

packages/mdc-tab-indicator/fading-foundation.js

+  }
+
+  activate() {
+    this.adapter_.registerEventHandler('transitionend', this.handleTransitionEnd_);


Its a little confusing that you re-register every time activate is called.

I realized after looking at MDCTabIndicatorFoundation that super. handleTransitionEnd() de-registers the listener....but thats also confusing because now that super. handleTransitionEnd() has a side-effect the sub-class is depending on.

I think you can replace super.handleTransitionEnd with simply this.adapter_.deregisterEventHandler('transitionend', this.handleTransitionEnd_);. You will end up copy/pasting the same line of code into the two subclasses, but I think thats better than having side-effects.

It will also remove the need for MDCTabIndicatorFoundation to have a handleTransitionEnd() method. MDCTabIndicatorFoundation will basically only have a computeClientRect() method.

lynnmercier · 2018-04-05T23:40:56Z

packages/mdc-tab-indicator/index.js

+      return new MDCFadingTabIndicatorFoundation(adapter);
+    }
+
+    // Default to the bar indicator


Default to the sliding indicator

lynnmercier · 2018-04-05T23:48:55Z

packages/mdc-tab-indicator/sliding-foundation.js

+    this.adapter_.setStyleProperty('transform', `translateX(${xPosition}px) scaleX(${widthDelta})`);
+
+    // Force repaint
+    this.computeClientRect();


works fine on chrome without this line, i think you might be able to remove it

This is necessary for Firefox. Another option would be to wrap the requestAnimationFrame in another requestAnimationFrame. Looks weird but at least hides fewer side effects.

lynnmercier · 2018-04-09T16:32:25Z

demos/tab-indicator.scss

-  flex: 1 0 auto;
-  width: 150px;
-  max-width: 150px;
+  @include mdc-tab-indicator-surface;


so will this mdc-tab-indicator-surface go on mdc-tab?

lynnmercier · 2018-04-09T16:41:59Z

packages/mdc-tab-indicator/_mixins.scss

 }

 @mixin mdc-tab-indicator-bar-color($color) {
-  .mdc-tab-indicator__bar {
+  &.mdc-tab-indicator--bar {


I think you should assume the client calls mdc-tab-indicator-bar-color on a tab indicator which is --bar. You don't need the &.mdc-tab-indicator--bar. In fact it could lead to overly specific CSS.

Same for below

Sounds good.

We'll need more specificity for the icon-height mixin, otherwise we're reliant on load order for font-size. Material Icons explicitly sets the font-size to 24px.

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)

patrickrodee added 30 commits March 9, 2018 18:57

WIP start of mdc-tab

1b3bac3

WIP updated with "hasClass" on foundation

8bd43c0

WIP testing

9d33e3b

WIP added tests; updated styles for custom indicator

0bbfe73

WIP adding demo

d531675

WIP more progess

8753c18

WIP demo

7fe42d5

WIP added material-icons because i'm a fool

9f1053c

WIP got ripples and custom theme working; need to update layout and RTL

2205fcb

WIP made mdc-tab private

dbb6a3d

WIP added readme; fixed up demo to look nice

0feb3ab

WIP added ripple exposure from MDCTab

f1c3097

WIP fixed spelling of "event"

52187c1

WIP added ripple foundation back to checkbox

3cc11f3

WIP updated all the tests...

0f2e7fa

WIP merged master

2cc6387

WIP cleaned up tab mixins file

b6b7380

WIP rewrote ripple test

d130f43

WIP re-rewrote ripple test

22a5656

WIP re-re-rewrote ripple test and implementation

047a45f

WIP changed ripple import order

a016ecf

WIP moved getters/setters

3744249

WIP changed static method order

f15e991

WIP upgraded tab to 0.33.0; upgraded ripple in tab to 0.33.0

1c0d1cc

Merge branch 'master' into feat/tabs/tab

fa5be74

WIP starting

5fa0260

WIP merge master

dab80ce

WIP progress

dd8554e

WIP progress

55d9f5e

WIP progress

88dae47

patrickrodee added 6 commits March 29, 2018 17:06

WIP got tests passing

6ba4fbe

WIP added demo for tab indicator

8603321

WIP updated demo; reworked icon visual centering

1ba59fc

WIP adding appendChild/removeChild to gBCR test

e015586

Merge branch 'master' into feat/tabs/tab-indicator

00af9db

WIP removed overrides from icon foundation

5841b72

acdvorak suggested changes Mar 30, 2018

View reviewed changes

lynnmercier suggested changes Mar 30, 2018

View reviewed changes

patrickrodee added 2 commits April 2, 2018 16:56

WIP updated from review

38c868f

WIP separating content from animation

164089a

patrickrodee changed the base branch from master to feat/tabs/tab April 3, 2018 18:46

patrickrodee changed the base branch from feat/tabs/tab to feat/tabs/tabs April 3, 2018 18:53

patrickrodee added 4 commits April 3, 2018 11:56

Merge branch 'feat/tabs/tabs' into feat/tabs/tab-indicator

8a41020

Merge branch 'feat/tabs/tab-indicator' of github.com:material-compone…

6b563c2

…nts/material-components-web into feat/tabs/tab-indicator

WIP updated from review

e4d6b76

WIP set travis to run for merges to feat/tabs/tabs

6e1b1a4

lynnmercier suggested changes Apr 5, 2018

View reviewed changes

patrickrodee added 3 commits April 6, 2018 15:46

WIP DONE

66b550f

WIP saved the day

058998e

WIP party time ✨

7aa0362

lynnmercier suggested changes Apr 9, 2018

View reviewed changes

WIP updated from review ✌️

c5ca744

lynnmercier approved these changes Apr 9, 2018

View reviewed changes

patrickrodee added 2 commits April 9, 2018 16:03

WIP renamed "bar" to "underline"

60463e4

WIP updated test names

ad27e5c

patrickrodee merged commit 914eca4 into feat/tabs/tabs Apr 9, 2018

acdvorak assigned lynnmercier Apr 9, 2018

trimox mentioned this pull request Jul 14, 2018

Add new MDC Tab Indicator trimox/angular-mdc-web#1117

Closed

kfranqueiro deleted the feat/tabs/tab-indicator branch August 1, 2018 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tab): Add tab indicator #2461

feat(tab): Add tab indicator #2461

patrickrodee commented Mar 26, 2018 •

edited

Loading

acdvorak Mar 30, 2018

acdvorak Mar 30, 2018

acdvorak Mar 30, 2018

acdvorak Mar 30, 2018

patrickrodee Mar 30, 2018 •

edited

Loading

lynnmercier left a comment

lynnmercier Mar 30, 2018

lynnmercier Mar 30, 2018

patrickrodee Apr 2, 2018

lynnmercier Mar 30, 2018

lynnmercier Mar 30, 2018

patrickrodee Apr 2, 2018

lynnmercier Mar 30, 2018

lynnmercier Mar 30, 2018

lynnmercier Mar 30, 2018

patrickrodee Apr 2, 2018

lynnmercier Mar 30, 2018

patrickrodee Apr 3, 2018

lynnmercier Apr 5, 2018

lynnmercier Apr 5, 2018

lynnmercier Apr 5, 2018

lynnmercier Apr 5, 2018

lynnmercier Apr 5, 2018

lynnmercier Apr 5, 2018

lynnmercier Apr 5, 2018

lynnmercier Apr 5, 2018

lynnmercier Apr 5, 2018

patrickrodee Apr 6, 2018

lynnmercier Apr 9, 2018

patrickrodee Apr 9, 2018

lynnmercier Apr 9, 2018

patrickrodee Apr 9, 2018


		#### Fading Icon Indicator

		We recommend you load [Material Icons](https://material.io/icons/) from Google Fonts. However, users are free to use whatever icons they like.


		@import "@material/theme/mixins";

		@mixin mdc-tab-indicator-active {

feat(tab): Add tab indicator #2461

feat(tab): Add tab indicator #2461

Conversation

patrickrodee commented Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickrodee Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

lynnmercier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickrodee commented Mar 26, 2018 •

edited

Loading

patrickrodee Mar 30, 2018 •

edited

Loading