feat(top-app-bar): Implement short top app bar #2290

williamernest · 2018-02-21T23:54:24Z

codecov-io · 2018-02-22T01:21:31Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a9f9bf2). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #2290   +/-   ##
=========================================
  Coverage          ?   99.08%           
=========================================
  Files             ?      100           
  Lines             ?     4046           
  Branches          ?      524           
=========================================
  Hits              ?     4009           
  Misses            ?       37           
  Partials          ?        0

Impacted Files	Coverage Δ
packages/mdc-top-app-bar/foundation.js	`100% <100%> (ø)`
packages/mdc-top-app-bar/constants.js	`100% <100%> (ø)`
packages/mdc-top-app-bar/util.js	`100% <100%> (ø)`
packages/mdc-top-app-bar/index.js	`100% <100%> (ø)`

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 a9f9bf2...1537127. Read the comment docs.

kfranqueiro · 2018-02-22T20:21:56Z

demos/top-app-bar.html

@@ -25,18 +25,29 @@
    <link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Roboto+Mono">
    <link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Roboto:300,400,500">
    <link rel="stylesheet" href="https://fonts.googleapis.com/icon?family=Material+Icons">
+    <style>


Why isn't this in demos/top-app-bar.scss?

kfranqueiro · 2018-02-22T20:36:24Z

demos/top-app-bar.html

@@ -137,6 +201,36 @@
        drawer.open = true;
      });

+      var iconSection = document.getElementById('iconSection');
+      var shortCheckbox = document.getElementById('short');
+      var oneRightIcon = document.getElementById('no-right-icon');


Variable name seems backwards of what this does?

kfranqueiro · 2018-02-22T20:36:49Z

demos/top-app-bar.html

+
+      oneRightIcon.addEventListener('change', function()  {
+        if(this.checked) {
+          iconSection.innerHTML = '';


Can we apply a class to the icon to set its display to none, rather than tweaking innerHTML? This doesn't seem like how we'd want anyone to actually do things.

Yes, but we'd have to change the totalActionItems adapter method to ignore display:none

kfranqueiro · 2018-02-22T20:46:11Z

packages/mdc-top-app-bar/README.md

@@ -95,6 +116,10 @@ Method Signature | Description
 `registerNavigationIconInteractionHandler(evtType: string, handler: EventListener) => void` | Registers an event listener on the native navigation icon element for a given event.
 `deregisterNavigationIconInteractionHandler(evtType: string, handler: EventListener) => void` | Deregisters an event listener on the native navigation icon element for a given event.
 `notifyNavigationIconClicked() => void` | Emits a custom event `MDCTopAppBar:nav` when the navigation icon is clicked. 
+`registerScrollHandler(handler) => void` | Registers a handler to be called when user scrolls. Our default implementation adds the handler as a listener to the window's `scroll` event.
+`deregisterScrollHandler(handler) => void` | Unregisters a handler to be called when user scrolls. Our default implementation removes the handler as a listener to the window's `scroll` event.
+`getViewportScrollY() => number` | Gets the number of pixels that the content of body is scrolled upward.


"upward" sounds potentially backwards here depending on how you think about it... how about "scrolled from the top of the page"?

kfranqueiro · 2018-02-22T20:46:32Z

packages/mdc-top-app-bar/README.md

+`registerScrollHandler(handler) => void` | Registers a handler to be called when user scrolls. Our default implementation adds the handler as a listener to the window's `scroll` event.
+`deregisterScrollHandler(handler) => void` | Unregisters a handler to be called when user scrolls. Our default implementation removes the handler as a listener to the window's `scroll` event.
+`getViewportScrollY() => number` | Gets the number of pixels that the content of body is scrolled upward.
+`totalActionIcons() => number` | Gets the number of action items on the right side of the top app bar.


We refer to action items in prose but the API uses the word Icons? Do we ever expect any action items that aren't icons?

Spec refers to them as Action Items but never uses anything other than icons. I'll change all of them to Items for consistency.

kfranqueiro · 2018-02-22T20:49:53Z

packages/mdc-top-app-bar/constants.js

@@ -17,9 +17,17 @@
 /** @enum {string} */
 const strings = {
  NAVIGATION_EVENT: 'MDCTopAppBar:nav',
+  TOPAPPBAR_SELECTOR: '.mdc-top-app-bar',


Rename to ROOT_SELECTOR (we have ROOT in other constants files for the top-level class)

kfranqueiro · 2018-02-22T20:51:58Z

packages/mdc-top-app-bar/mdc-top-app-bar.scss

@@ -36,13 +37,12 @@
    position: relative;
    box-sizing: border-box;
    width: 100%;
-    height: auto;
-    min-height: $mdc-top-app-bar-row-height;
+    height: $mdc-top-app-bar-row-height;


Curious why we had height + min-height earlier but now don't need it?

It was brought over from toolbar. I think it was used for flexible toolbars. I'll leave it in so we don't have to change it all when we get the flexible toolbar moved.

kfranqueiro · 2018-02-22T20:53:26Z

packages/mdc-top-app-bar/_variables.scss

+$mdc-top-app-bar-short-collapsed-width: 56px;
+$mdc-top-app-bar-short-collapsed-right-icon-padding: 12px;
+
+$mdc-top-app-bar-standard-easing: cubic-bezier(.4, 0, .2, 1);


Pretty sure this matches one of the variables in mdc-animation; reuse that instead?

kfranqueiro · 2018-02-23T22:07:24Z

packages/mdc-top-app-bar/README.md

+`registerScrollHandler(handler) => void` | Registers a handler to be called when user scrolls. Our default implementation adds the handler as a listener to the window's `scroll` event.
+`deregisterScrollHandler(handler) => void` | Unregisters a handler to be called when user scrolls. Our default implementation removes the handler as a listener to the window's `scroll` event.
+`getViewportScrollY() => number` | Gets the number of pixels that the content of body is scrolled from the top of the page.
+`totalActionIcons() => number` | Gets the number of action items on the right side of the top app bar.


Update to totalActionItems

kfranqueiro · 2018-02-23T22:08:20Z

demos/top-app-bar.html

+          appBar.destroy();
+          appBar = mdc.topAppBar.MDCTopAppBar.attachTo(appBarEl);
+
+          if(!this.checked){


Missing spaces around parens here and L208

kfranqueiro · 2018-02-23T22:09:22Z

demos/top-app-bar.html

+        });
+
+        shortCheckbox.addEventListener('change', function() {
+          this.checked ? appBarEl.classList.add('mdc-top-app-bar--short') : appBarEl.classList.remove('mdc-top-app-bar--short');


appBarEl.classList[this.checked ? 'add' : 'remove'](...);

(It's awkward to use a ternary for an entire statement.)

kfranqueiro · 2018-02-23T22:10:10Z

demos/top-app-bar.scss

+  margin: 0;
+  box-sizing: border-box;
+}
+.demo-main {


Add newline above this line

kfranqueiro · 2018-02-23T22:10:48Z

packages/mdc-top-app-bar/_mixins.scss

@@ -15,6 +15,7 @@
 //

 @import "@material/theme/variables"; // for mdc-theme-accessible-ink-color
+@import "@material/animation/variables";


Alphabetize these - move this and ripple above theme

kfranqueiro · 2018-02-23T22:12:26Z

packages/mdc-top-app-bar/constants.js

+/** @enum {string} */
+const cssClasses = {
+  SHORT_CLASS: 'mdc-top-app-bar--short',
+  RIGHT_ACTION_ITEM_CLASS: 'mdc-top-app-bar--short__right-action-item',


What's the meaning behind "right" action item, anyway? This term is probably a misnomer in RTL context so I'm wondering if there's something more semantic/agnostic we could use.

kfranqueiro · 2018-02-23T22:13:33Z

packages/mdc-top-app-bar/foundation.js

+      registerScrollHandler: (/* handler: EventListener */) => {},
+      deregisterScrollHandler: (/* handler: EventListener */) => {},
+      getViewportScrollY: () => /* number */ 0,
+      totalActionItems: () => /* number */ 0,


Sorry I realized this late, but this probably deserves to be called getTotalActionItems (verbs in function names are good)

kfranqueiro

The collapsed bar seems to have incorrect paddings in IE11 (the rightmost icon is always too far to the right, even in RTL).

kfranqueiro · 2018-02-26T17:39:22Z

demos/top-app-bar.html

+      </div>
+      <div>
+        <input type="checkbox" id="no-right-item-checkbox"/>
+        <label for="no-right-item-checkbox">No Right Item</label>


Nit: reword to "No Action Item"?

kfranqueiro · 2018-02-26T17:46:58Z

packages/mdc-top-app-bar/README.md

+`registerScrollHandler(handler) => void` | Registers a handler to be called when user scrolls. Our default implementation adds the handler as a listener to the window's `scroll` event.
+`deregisterScrollHandler(handler) => void` | Unregisters a handler to be called when user scrolls. Our default implementation removes the handler as a listener to the window's `scroll` event.
+`getViewportScrollY() => number` | Gets the number of pixels that the content of body is scrolled from the top of the page.
+`getTotalActionItems() => number` | Gets the number of action items on the top app bar.


Nit: on -> in?

kfranqueiro · 2018-02-26T17:48:39Z

packages/mdc-top-app-bar/_mixins.scss

+  @include mdc-rtl-reflexive_(border-bottom-left-radius, 0, border-bottom-right-radius, $mdc-top-app-bar-short-collapsed-border-radius);
+}
+
+@mixin mdc-top-app-bar-mobile-breakpoint($mobile-breakpoint: $mdc-top-app-bar-mobile-breakpoint) {


Should we make this a private mixin and add !default to the breakpoint variable instead? Otherwise these styles are getting emitted for the default breakpoint anyway and there's not really a way to undo that.

kfranqueiro · 2018-02-26T17:49:38Z

packages/mdc-top-app-bar/constants.js

+const cssClasses = {
+  SHORT_CLASS: 'mdc-top-app-bar--short',
+  SHORT_HAS_ACTION_ITEM_CLASS: 'mdc-top-app-bar--short-has-action-item',
+  SHORT_CLOSED_CLASS: 'mdc-top-app-bar--short-collapsed',


Rename to SHORT_COLLAPSED_CLASS if we're referring to the state as collapsed in the JS anyway?

kfranqueiro · 2018-02-26T17:50:55Z

packages/mdc-top-app-bar/foundation.js

+  /**
+   * Used to set the initial style of the short top app bar
+   */
+  styleShortAppBar() {


Is there a reason why this API is public? (We're not even using it in the demo, but that's kind of a special case anyway; we normally wouldn't expect people to switch the type of app bar dynamically.)

kfranqueiro · 2018-02-26T18:05:43Z

test/unit/mdc-top-app-bar/foundation.test.js

+  td.verify(mockAdapter.registerScrollHandler(td.matchers.isA(Function)), {times: 1});
+});
+
+test('short top app bar: on scroll listener is removed on destroy', () => {


Nit: Remove "on" before "scroll"

kfranqueiro · 2018-02-26T18:06:55Z

test/unit/mdc-top-app-bar/foundation.test.js

+  td.verify(mockAdapter.addClass(MDCTopAppBarFoundation.cssClasses.SHORT_HAS_ACTION_ITEM_CLASS), {times: 1});
+});
+
+test('short top app bar: class is not added if it has an action item', () => {


"if it does not have an action item"

kfranqueiro · 2018-02-26T18:08:21Z

test/unit/mdc-top-app-bar/mdc-top-app-bar.test.js

@@ -140,6 +142,14 @@ test('#adapter.registerNavigationIconInteractionHandler adds a handler to the na
  td.verify(handler(td.matchers.anything()));
 });

+test('#adapter.deregisterScrollHandler does not remove a handler to the nav icon if the nav icon is null ', () => {


kfranqueiro · 2018-02-26T18:09:13Z

test/unit/mdc-top-app-bar/mdc-top-app-bar.test.js

+  component.getDefaultFoundation().adapter_.deregisterScrollHandler(handler);
+  domEvents.emit(window, 'scroll');
+  try {
+    td.verify(handler(td.matchers.anything()), {times: 0});


Is this still going to properly fail the test case if we catch it?

We're not catching the error, we're just using a finally to ensure the scroll handler gets unregistered from the window. I've checked that the test fails if the deregister fails.

kfranqueiro · 2018-02-26T18:11:17Z

test/unit/mdc-top-app-bar/util.test.js

+      },
+    },
+  };
+  assert.isNotOk(util.applyPassive(mockWindow, true));


isNotOk -> isFalse

kfranqueiro · 2018-02-28T17:52:06Z

test/unit/mdc-top-app-bar/foundation.test.js

+  td.when(mockAdapter.hasClass(MDCTopAppBarFoundation.cssClasses.SHORT_CLASS)).thenReturn(true);
+  foundation.init();
+  foundation.destroy();
+  td.verify(mockAdapter.registerScrollHandler(td.matchers.isA(Function)), {times: 1});


This is supposed to be testing deregister, right?

kfranqueiro · 2018-02-28T17:53:13Z

test/unit/mdc-top-app-bar/foundation.test.js

+  td.when(mockAdapter.hasClass(MDCTopAppBarFoundation.cssClasses.SHORT_CLASS)).thenReturn(true);
+  td.when(mockAdapter.getTotalActionItems()).thenReturn(0);
+  foundation.init();
+  td.verify(mockAdapter.addClass(MDCTopAppBarFoundation.cssClasses.SHORT_HAS_ACTION_ITEM_CLASS), {times: 0});


Our impl is actually removing this class in this case, right? We should test that removeClass is invoked.

We're testing that this class was never added. It's added during init if the html has an action item.

kfranqueiro · 2018-02-28T17:54:24Z

test/unit/mdc-top-app-bar/mdc-top-app-bar.test.js

+  assert.equal(component.getDefaultFoundation().adapter_.getViewportScrollY(), window.pageYOffset);
+});
+
+test('adapter#getTotalActionItems returns the amount of action icons on the opposite side of the menu', () => {


amount -> number, icons -> items?

@alternate

commit f17e0d3 Author: Will Ernest <34519388+williamernest@users.noreply.github.com> Date: Thu Mar 8 13:20:48 2018 -0500 fix(text-field): Clicking label should focus textfield (#2353) commit 77b15f4 Author: Bonnie Zhou <bzbee003@gmail.com> Date: Wed Mar 7 16:29:57 2018 -0800 refactor(button): Remove compact variant (#2361) BREAKING CHANGE: The compact variant of MDC Button is removed. commit 35a5cfc Author: Will Ernest <34519388+williamernest@users.noreply.github.com> Date: Tue Mar 6 19:54:23 2018 -0500 fix(toolbar): Fix colors for svg icons. Update custom-toolbar demo (#2331) SVG icons in the toolbar will now use the same color as the font icons. commit dc52201 Author: Andrew C. Dvorak <acdvorak@gmail.com> Date: Tue Mar 6 16:00:12 2018 -0800 fix(theme): Move @alternate annotations for Closure Stylesheets (#2355) `@alternate` annotations need to come before the _second_ property. Otherwise, Closure Compiler strips the first property (it does not output it at all). commit 3c04419 Author: aprigogin <17075403+aprigogin@users.noreply.github.com> Date: Tue Mar 6 14:29:12 2018 -0800 fix(button): icon in rtl should have margin right flipped. (#2346) commit b9000a4 Author: Cory Reed <swashcap@gmail.com> Date: Tue Mar 6 07:37:40 2018 -0800 fix(demos): Correct RTL/LTR toggling in demos in Safari (#2348) commit ab85736 Author: Andrew C. Dvorak <acdvorak@gmail.com> Date: Mon Mar 5 19:00:11 2018 -0500 fix: Use `var` instead of `const` in menu demo (#2345) Safari 9.x and IE 10 do not support `const` commit dc3d69f Author: aprigogin <17075403+aprigogin@users.noreply.github.com> Date: Mon Mar 5 15:22:31 2018 -0800 fix(rtl): Adding noflip annotations to fix downstream rtl issues (#2344) * fix(rtl): Adding noflip annotations to fix downstream rtl issues * fix(rtl): remove changes to button, will be in separate PR * fix(rtl): remove changes to button, will be in separate PR - second attempt. * fix(rtl): removed extra whitespace commit eb4138e Author: Patrick RoDee <prodee@google.com> Date: Mon Mar 5 14:10:46 2018 -0800 docs: Update CHANGELOG.md commit f478610 Author: Patrick RoDee <prodee@google.com> Date: Mon Mar 5 14:10:30 2018 -0800 chore: Publish commit 78408bb Author: Andrew C. Dvorak <acdvorak@gmail.com> Date: Mon Mar 5 16:13:02 2018 -0500 fix: Use `var` instead of `const` in demos/ready.js (#2343) Safari 9.x and IE 10 do not support `const`, and `ready.js` is not transpiled to ES5. commit 49a9449 Author: Will Ernest <34519388+williamernest@users.noreply.github.com> Date: Mon Mar 5 12:21:54 2018 -0500 chore(select): Fix JS example in Readme (#2332) commit bc17291 Author: Will Ernest <34519388+williamernest@users.noreply.github.com> Date: Fri Mar 2 13:21:07 2018 -0500 feat(top app bar): Add short top app bar always collapsed feature (#2327) commit 0ba7d10 Author: Matty Goo <mattgoo@google.com> Date: Fri Mar 2 11:33:19 2018 -0500 fix(text-field): disable validation check in setRequired (#2201) BREAKING CHANGE: removed setRequired and isRequired from foundation. commit c14d244 Author: Will Ernest <34519388+williamernest@users.noreply.github.com> Date: Fri Mar 2 10:37:18 2018 -0500 chore(select): Fix ID values in demo (#2330) Remove unused ID and changed duplicated ID values. Also fixed the parentheses in RTL. commit ecf4060 Author: Bonnie Zhou <bzbee003@gmail.com> Date: Thu Mar 1 16:38:41 2018 -0500 feat(chips): Change chip color when selected (#2329) BREAKING CHANGE: The `mdc-chip--activated` class, `mdc-chip-activated-ink-color` Sass mixin, and the `toggleActive` methods on `MDCChip`/`MDCChipSet` have been renamed to `mdc-chip--selected`, `mdc-chip-selected-ink-color`, and `toggleSelected`, respectively. commit 4b24b51 Author: Matty Goo <mattgoo@google.com> Date: Wed Feb 28 15:20:19 2018 -0500 chore(floating-label): separate label module from text-field (#2237) BREAKING CHANGE: must use `.mdc-floating-label` selector instead of `.mdc-text-field__label` commit fd8d8d9 Author: Will Ernest <34519388+williamernest@users.noreply.github.com> Date: Wed Feb 28 14:21:55 2018 -0500 feat(top-app-bar): Implement short top app bar (#2290) Adds the short top app bar variant to the top app bar. commit a9f9bf2 Author: Kenneth G. Franqueiro <kfranqueiro@users.noreply.github.com> Date: Wed Feb 28 13:06:41 2018 -0500 docs(select): Fix front matter for label sub-component (#2323)

feat(top-app-bar): Implement short top app bar

52b1b49

williamernest added the do-not-merge label Feb 21, 2018

williamernest added 4 commits February 21, 2018 16:23

feat(top-app-bar): Add rtl support. Update readme.

7bf080b

feat(top-app-bar): Update demo for rtl

bd45978

feat(top-app-bar): Update tests

08f3cb7

feat(top-app-bar): Update to catch an edge case

6de87c1

williamernest removed the do-not-merge label Feb 22, 2018

williamernest requested a review from kfranqueiro February 22, 2018 01:12

williamernest assigned kfranqueiro Feb 22, 2018

Merge branch 'master' into feat/top-app-bar/add-short-top-app-bar

7271757

kfranqueiro suggested changes Feb 22, 2018

View reviewed changes

williamernest added 5 commits February 22, 2018 14:28

feat(top-app-bar): Update for comments

4709d6f

feat(top-app-bar): Rename adapter method

cc4139c

feat(top-app-bar): Fix class name in readme.

df9503d

feat(top-app-bar): Remove display none from adapter code

4706e5e

feat(top-app-bar): Rename mixin class selector to new name

437586f

kfranqueiro suggested changes Feb 23, 2018

View reviewed changes

feat(top-app-bar): Update for comments

a9f1749

williamernest force-pushed the feat/top-app-bar/add-short-top-app-bar branch from 6fd2e5e to a9f1749 Compare February 23, 2018 22:57

williamernest and others added 2 commits February 23, 2018 15:56

feat(top-app-bar): Update for comments. Update tests/readme

5c7bfda

Merge branch 'master' into feat/top-app-bar/add-short-top-app-bar

d56805f

kfranqueiro suggested changes Feb 26, 2018

View reviewed changes

feat(top-app-bar): Update for comments

71324b5

williamernest force-pushed the feat/top-app-bar/add-short-top-app-bar branch 6 times, most recently from 5b38e44 to 1e2d75f Compare February 27, 2018 19:12

feat(top-app-bar): Fix IE11/Edge

b359062

williamernest force-pushed the feat/top-app-bar/add-short-top-app-bar branch from 1e2d75f to b359062 Compare February 27, 2018 19:29

williamernest and others added 3 commits February 27, 2018 15:44

feat(top-app-bar): Update for lint

e9df173

Merge branch 'master' into feat/top-app-bar/add-short-top-app-bar

97bb259

feat(top-app-bar): Update for comments

6f59ec6

kfranqueiro suggested changes Feb 28, 2018

View reviewed changes

feat(top-app-bar): Update for comments

61c572b

kfranqueiro approved these changes Feb 28, 2018

View reviewed changes

Merge branch 'master' into feat/top-app-bar/add-short-top-app-bar

1537127

williamernest merged commit fd8d8d9 into master Feb 28, 2018

williamernest deleted the feat/top-app-bar/add-short-top-app-bar branch February 28, 2018 19:21

feat(top-app-bar): Implement short top app bar #2290

feat(top-app-bar): Implement short top app bar #2290

Conversation

williamernest commented Feb 21, 2018

codecov-io commented Feb 22, 2018 • edited Loading

Codecov Report

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

kfranqueiro 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

codecov-io commented Feb 22, 2018 •

edited

Loading