feat(top-app-bar): Add top app bar component #2225

williamernest · 2018-02-10T00:45:44Z

codecov-io · 2018-02-12T16:58:19Z

Codecov Report

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

@@            Coverage Diff            @@
##             master    #2225   +/-   ##
=========================================
  Coverage          ?   98.73%           
=========================================
  Files             ?       96           
  Lines             ?     3943           
  Branches          ?      508           
=========================================
  Hits              ?     3893           
  Misses            ?       50           
  Partials          ?        0

Impacted Files	Coverage Δ
packages/mdc-top-app-bar/foundation.js	`100% <100%> (ø)`
packages/mdc-top-app-bar/index.js	`100% <100%> (ø)`
packages/mdc-top-app-bar/constants.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 f89cd10...dac8de5. Read the comment docs.

kfranqueiro · 2018-02-14T22:19:49Z

demos/index.html

@@ -215,6 +215,14 @@
            </span>
          </a>

+          <a href="top-app-bar.html" role="listitem" class="mdc-list-item">


p comes after o, so this should be after Toolbar if we're being consistent with alpha order.

OTOH I talked with Will about potential for confusion by adding this alongside toolbar before there are features implemented that differentiate it (and before we eventually relegate the toolbar package for different functionality...) and maybe we just shouldn't add this to the demo page until the distinguishing features are added? (So maybe we shouldn't add front-matter to the readme for the site yet either.)

kfranqueiro · 2018-02-14T22:59:20Z

packages/mdc-top-app-bar/package-lock.json

@@ -0,0 +1,18 @@
+{


We don't have package-lock.json within any of our other packages folders, so I don't think this needs to be committed? (Or we should commit them across the board at some point later, but i.e. I don't think they get generated when doing npm install from the top-level which is usually all we need to do for development.)

kfranqueiro · 2018-02-14T22:59:40Z

demos/top-app-bar.scss

+  }
+}
+
+.demo-controls-container {


I don't see this used anywhere?

kfranqueiro · 2018-02-14T23:00:21Z

package-lock.json

@@ -2,6 +2,16 @@
  "requires": true,
  "lockfileVersion": 1,
  "dependencies": {
+    "JSONStream": {


All of these changes seem to just reorder things, so I'm not sure this is needed in this PR?

kfranqueiro · 2018-02-14T23:00:52Z

package.json

@@ -151,6 +151,7 @@
          "text-field",
          "theme",
          "toolbar",
+          "top-app-bar",


What about closure whitelist?

toolbar doesn't currently exist in closure whitelist, so I didn't add this one either.

I think any component we are writing from scratch should aim to be in the whitelist at the get-go at this point, because it's easier to deal with incrementally than having to come back through and clean up later.

kfranqueiro · 2018-02-14T23:18:06Z

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

+`mdc-top-app-bar-ink-color($color)` | Sets the ink color of the top app bar
+`mdc-top-app-bar-fill-color($color)` | Sets the fill color of the top app bar
+`mdc-top-app-bar-fill-color-accessible($color)` | Sets the fill color of the top app bar and automatically sets a high-contrast ink color
+`mdc-top-app-bar-icon-ink-color($color)` | Sets the ink color of a top app bar icon


"of top app bar icons"?

kfranqueiro · 2018-02-14T23:19:25Z

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

+@mixin mdc-top-app-bar-icon-ink-color($color) {
+  .mdc-top-app-bar__icon,
+  .mdc-top-app-bar__menu-icon {
+    @include mdc-theme-prop(color, $color);


I would leave mdc-states out of the private mixin, and include it here and pass $color. (Edit: this was done)

kfranqueiro · 2018-02-14T23:19:56Z

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

+/* eslint no-unused-vars: [2, {"args": "none"}] */
+
+/**
+ * Adapter for MDC Top App Bar .


Remove space before period

kfranqueiro · 2018-02-14T23:20:35Z

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

+  removeClass(className) {}
+
+  /**
+   * Returns true if the root element contains the given class name.


Capitalize Element to be consistent with above, or lowercase it everywhere?

kfranqueiro · 2018-02-14T23:22:27Z

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

+   */
+  static get defaultAdapter() {
+    return /** @type {!MDCTopAppBarAdapter} */ {
+      hasClass: (/* className: string */) => /* boolean */ false,


Pretty sure this should be {} on the RHS, as in, it's an empty function (not a return value).

I realize we seem to be 50/50 on this throughout the repo currently but I'm inclined to think it's a mistake.

It was copied from the toolbar.

kfranqueiro · 2018-02-14T23:39:46Z

To explain what's going on here in case anyone's wondering, we're planning on reorganizing toolbar into common logic + specializations. Since mdc-toolbar was originally created primarily with scenarios for top app bars in mind, it's likely that some functionality currently present in mdc-toolbar will eventually manifest here and be removed from the mdc-toolbar package.

kfranqueiro

FYI I found a bug: It seems like everything in the top app bar is vertically misaligned too low in Firefox, Edge, and IE.

kfranqueiro · 2018-02-15T22:42:20Z

demos/top-app-bar.html

+      </div>
+    </main>
+
+    <script src="/assets/material-components-web.js" async></script>


Load /assets/common.js above this line, to automatically get preventDefault on a elements (to avoid the browser navigating to #)

kfranqueiro · 2018-02-15T22:50:25Z

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

+section: components
+excerpt: "A container for multiple rows of items such as application title, navigation menu, or tabs."
+iconId: toolbar
+path: /catalog/topappbar/


If we're adding this front-matter now, I would hyphenate top-app-bar here just like the package name.

kfranqueiro · 2018-02-15T22:50:51Z

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

@@ -0,0 +1,112 @@
+<!--docs:
+title: "Toolbars"


Update title if we're adding this front-matter now

kfranqueiro · 2018-02-15T22:51:37Z

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

+
+# Top App Bar
+
+MDC Top App Bar acts as a container for items such as


Nit: the line wrapping here seems weird

kfranqueiro · 2018-02-15T22:53:19Z

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

+```html
+<header class="mdc-top-app-bar">
+  <div class="mdc-top-app-bar__row">
+    <section class="mdc-top-app-bar__section mdc-top-app-bar__section--align-start">


Out of curiosity... does design have more semantic names we should be using for the align-start and align-end sections? Or are they general-purpose enough that there's no name that would fit all cases?

I don't think designers know we split the toolbar into two sides so we can show some things on the left and some things on the right. I'll get with the designer to see if they have any input bit it seems like this was named by a developer smashing together align-items and flex-start. Which makes sense for developers to use.

kfranqueiro · 2018-02-15T23:21:36Z

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

+  }
+
+  &__icon,
+  &__menu-icon {


Thank you for grouping these two together to reduce CSS output <3

kfranqueiro · 2018-02-15T23:25:21Z

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

+  removeClass(className) {}
+
+  /**
+   * Returns true if the root Element contains the given class name.


Nit: We say just "class" in the other two descriptions but "class name" here (and "className" in the readme)... should we just reduce this description to class as well?

kfranqueiro · 2018-02-15T23:26:50Z

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

+    const icons = [].slice.call(this.root_.querySelectorAll(strings.ACTION_ICON_SELECTOR));
+    icons.push(this.navIcon_);
+
+    this.iconRipples_ = icons.map(function(icon) {


You can use fat arrow here and in the forEach below

kfranqueiro · 2018-02-15T23:38:10Z

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

+    this.navIconClick_ = this.navigationEvent_.bind(this);
+
+    if (this.navIcon_) {
+      this.navIcon_.addEventListener('click', this.navIconClick_);


This much logic should not be in the component. This should be hooked up through the foundation with [de]registerInteractionHandler APIs.

Look at textfield/icon for example (not saying this has to be a subcomponent, but that shows an example of hooking up icon interaction + event emit through the foundation and that's almost the only thing that one does).

kfranqueiro · 2018-02-15T23:39:48Z

packages/mdc-top-app-bar/package.json

+  ],
+  "dependencies": {
+    "@material/base": "^0.29.0",
+    "@material/elevation": "^0.28.0",


I guess we're planning to use this later? It's not currently used, seemingly.

Yeah, this is a remnant of my prototype.

kfranqueiro · 2018-02-16T19:07:19Z

demos/top-app-bar.scss

+  padding: 20px 28px;
+}
+
+@media (max-width: 599px) {


Reference $mdc-top-app-bar-mobile-breakpoint ?

kfranqueiro · 2018-02-16T19:20:18Z

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

+// limitations under the License.
+//
+
+@import "@material/elevation/mixins";


I flagged this in package.json earlier, and since we removed it there, we should remove it here to avoid breaking things.

kfranqueiro · 2018-02-16T19:21:01Z

demos/top-app-bar.scss

+@import "../packages/mdc-button/mdc-button";
+@import "../packages/mdc-list/mdc-list";
+@import "../packages/mdc-menu/mdc-menu";
+@import "../packages/mdc-theme/color-palette";


Unused import?

kfranqueiro · 2018-02-16T19:21:34Z

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

+// limitations under the License.
+//
+
+@import "@material/theme/variables"; // for mdc-theme-accessible-ink-color


Technically missing theme/mixins for mdc-theme-prop?

kfranqueiro · 2018-02-16T19:25:10Z

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

+
+@import "@material/elevation/mixins";
+@import "@material/rtl/mixins";
+@import "@material/theme/mixins";


I don't think theme/mixins is needed in this file? (I commented about it being missing in the mixins file though.)

kfranqueiro · 2018-02-16T19:27:39Z

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

+test('#init calls component event registrations', () => {
+  const {foundation} = setupTest();
+
+  foundation.init();


This isn't testing anything? Should this be mocking adapter#registerNavigationInteractionHandler?

There's also no reciprocal test for #destroy and deregister.

kfranqueiro · 2018-02-16T19:29:05Z

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

+  foundation.init();
+});
+
+test('foundation returns strings', () => {


Why not combine this with the "exports strings" test? e.g.

material-components-web/test/unit/mdc-checkbox/foundation.test.js

Line 100 in 1cc5dd5

assert.deepEqual(MDCCheckboxFoundation.strings, strings);

kfranqueiro · 2018-02-16T19:31:22Z

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

+
+  let click;
+
+  td.when(mockAdapter.registerNavigationIconInteractionHandler('click', td.matchers.isA(Function)))


You might be able to use captureHandlers in test/unit/helpers/foundation.js to help with this.

See e.g. how ripple uses it:

material-components-web/test/unit/mdc-ripple/foundation-activation.test.js

Line 27 in 1cc5dd5

const handlers = captureHandlers(adapter, 'registerInteractionHandler');

kfranqueiro · 2018-02-16T19:32:30Z

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

+test('adapter#hasClass returns true if the root element has specified class', () => {
+  const {root, component} = setupTest();
+  root.classList.add('foo');
+  assert.isOk(component.getDefaultFoundation().adapter_.hasClass('foo'));


Nit: I know we do this a bunch of places, but I'd advocate using isTrue and isFalse specifically given that the API is supposed to return a boolean.

kfranqueiro · 2018-02-16T19:48:48Z

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

@@ -0,0 +1,104 @@
+


Remove leading newline?

acdvorak

Goldens and css-bundle-factory LGTM!

kfranqueiro

I'm noticing that in seemingly every browser except chrome, the ripples are not positioned/sized correctly. This might be a case of missing position: relative on the surface, which we've run into before (and I should really look at the techdebt issue of centralizing...).

kfranqueiro · 2018-02-20T22:40:10Z

demos/top-app-bar.html

+    <meta name="viewport" content="width=device-width, initial-scale=1">
+    <link rel="icon" type="image/png" href="/images/logo_components_color_2x_web_48dp.png">
+    <link rel="stylesheet" href="/assets/top-app-bar.css">
+    <link rel="stylesheet" href="/assets/drawer/drawer.css">


Not sure how I feel about this vs. importing mdc-drawer in the top-app-bar demo scss. Including this instead also picks up a bunch of demo styles specific to the drawer page that aren't being used here.

kfranqueiro · 2018-02-20T22:41:43Z

demos/top-app-bar.html

+      </div>
+    </header>
+
+    <aside class="mdc-drawer mdc-drawer--temporary demo-drawer">


I don't think the demo-drawer and demo-drawer-list-item classes are being styled at all in this demo page.

kfranqueiro · 2018-02-20T22:42:14Z

demos/top-app-bar.html

+    <script src="/assets/material-components-web.js" async></script>
+    <script type="text/javascript">
+    demoReady(function() {
+      var appBarElement = document.getElementById('demo-top-app-bar');


Nit: Shorten to appBarEl to match drawerEl?

kfranqueiro · 2018-02-20T22:55:01Z

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

+`removeClass(className: string) => void` | Removes a class from the root element of the component.
+`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. 


Nit: switch from quotes to backticks around the custom event name

kfranqueiro

FYI we currently have no tests for the top-app-bar component's destroy logic (to iterate and destroy ripples), but that might require adding a factory argument along the lines of what textfield/index.js does. I commented on a couple of other things we can probably test more easily to increase coverage.

kfranqueiro · 2018-02-21T20:42:49Z

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

+
+Event Name | Event Data Structure | Description
+--- | --- | ---
+`MDCTopAppBar:nav` | None | Emits when the menu icon is clicked.


Nit: we say "navigation icon" a few lines up, but "menu icon" here... is there one right word for it?

kfranqueiro · 2018-02-21T20:43:54Z

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

+/** @enum {string} */
+const strings = {
+  NAVIGATION_EVENT: 'MDCTopAppBar:nav',
+  TITLE_SELECTOR: '.mdc-top-app-bar__title',


Is this used (or will it be)?

It will be used in the next PR. It won't hurt to leave it in for now.

kfranqueiro · 2018-02-21T20:54:07Z

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

+  assert.isFalse(root.classList.contains('foo'));
+});
+
+test('#adapter.registerNavigationIconInteractionHandler adds a handler to the nav icon ' +


How hard would it be to add tests to verify that no handler gets registered if there is no navigation icon? Currently the implicit "else" branch isn't covered in the register/deregister logic.

kfranqueiro · 2018-02-21T22:21:59Z

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

+  return {root, adjust, component, icon};
+}
+
+class FakeRipple {


Can we move this above setupTest? It's jarring to see things defined after they're used.

kfranqueiro · 2018-02-21T22:22:49Z

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

 }

 suite('MDCTopAppBar');

-test('attachTo initializes and returns an MDCTopAppBar instance', () => {
+test('initialize and returns an MDCTopAppBar instance', () => {


Why was this test name rephrased? It still does the same thing (and now it's grammatically incorrect).

kfranqueiro · 2018-02-21T22:25:05Z

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

  assert.isTrue(MDCTopAppBar.attachTo(getFixture()) instanceof MDCTopAppBar);
 });

+test('constructor instantiates icon ripples', () => {
+  const {root, component} = setupTest();
+  const icons = root.querySelectorAll('.mdc-top-app-bar__icon').length + 1;


Can this reuse the two *_ICON_SELECTOR constants together in one selector and forego the +1?

kfranqueiro

LGTM provided that CI passes

feat(top-app-bar): Initial component

308a4d6

williamernest requested a review from lynnmercier February 10, 2018 00:47

feat(top-app-bar): Remove package-lock.json

d4ab6a1

feat(top-app-bar): Update test

8829dc3

williamernest force-pushed the feat/top-app-bar/new-component branch from 254d145 to 8829dc3 Compare February 12, 2018 17:31

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

williamernest and others added 2 commits February 12, 2018 12:11

feat(top-app-bar): Add icon states

79958fc

Merge branch 'master' into feat/top-app-bar/new-component

e96e8f5

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

williamernest and others added 4 commits February 12, 2018 14:54

feat(top-app-bar): Remove unused styles/file

472d678

feat(top-app-bar): Update demo top item of drawer to be back button

faff808

feat(top-app-bar): Add mobile default styles

e50529c

Merge branch 'master' into feat/top-app-bar/new-component

e477c9e

acdvorak assigned kfranqueiro Feb 13, 2018

williamernest added 2 commits February 14, 2018 08:41

feat(top-app-bar): Clean up Component initialize function

b965678

feat(top-app-bar): Merge master

18b5d4c

kfranqueiro suggested changes Feb 14, 2018

View reviewed changes

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

dc4049c

williamernest added 2 commits February 14, 2018 16:51

temp

9778668

WIP: Work on closure

5b47a3f

williamernest force-pushed the feat/top-app-bar/new-component branch from be4a5db to 5b47a3f Compare February 15, 2018 17:52

williamernest and others added 4 commits February 15, 2018 12:55

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

ecfe982

feat(top-app-bar): Fix closure

61a8519

Merge branch 'master' into feat/top-app-bar/new-component

a9723b4

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

d6d4e46

kfranqueiro suggested changes Feb 15, 2018

View reviewed changes

williamernest added 2 commits February 15, 2018 16:08

Merge master

ded552c

feat(top-app-bar): Resolve comments

7083bbc

williamernest removed the request for review from lynnmercier February 16, 2018 15:48

williamernest added 2 commits February 16, 2018 09:41

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

bff6e07

feat(top-app-bar): Update demo

68323ca

kfranqueiro suggested changes Feb 16, 2018

View reviewed changes

kfranqueiro reviewed Feb 16, 2018

View reviewed changes

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

@@ -0,0 +1,104 @@

Copy link

Contributor

kfranqueiro Feb 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove leading newline?

acdvorak reviewed Feb 16, 2018

View reviewed changes

williamernest added 2 commits February 16, 2018 14:00

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

95c7125

feat(top-app-bar): Consolidate for strings test

ca70eb4

williamernest force-pushed the feat/top-app-bar/new-component branch from b7dcacf to ca70eb4 Compare February 16, 2018 22:23

williamernest and others added 2 commits February 16, 2018 15:18

feat(top-app-bar): Remove blank line

9e437f2

Merge branch 'master' into feat/top-app-bar/new-component

b0a46dc

kfranqueiro suggested changes Feb 20, 2018

View reviewed changes

kfranqueiro reviewed Feb 20, 2018

View reviewed changes

williamernest and others added 5 commits February 20, 2018 16:06

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

3388a58

Merge branch 'master' into feat/top-app-bar/new-component

fd75203

feat(top-app-bar): Update deps for release

bdf2023

feat(top-app-bar): Add outline none to icons

09a8d2f

feat(top-app-bar): Add height/width to icons to prevent race condition

34735fe

williamernest force-pushed the feat/top-app-bar/new-component branch from 7875c6e to 34735fe Compare February 21, 2018 19:38

kfranqueiro suggested changes Feb 21, 2018

View reviewed changes

feat(top-app-bar): Resolve comments

8659a6f

kfranqueiro suggested changes Feb 21, 2018

View reviewed changes

williamernest and others added 3 commits February 21, 2018 14:31

feat(top-app-bar): Update tests

87b329d

Merge branch 'master' into feat/top-app-bar/new-component

af14a82

feat(top-app-bar): Update test description

dac8de5

kfranqueiro approved these changes Feb 21, 2018

View reviewed changes

williamernest merged commit 0ad69c4 into master Feb 21, 2018

williamernest deleted the feat/top-app-bar/new-component branch February 21, 2018 23:07

williamernest mentioned this pull request Mar 1, 2018

Create a new mdc-top-app-bar #2222

Closed

4 tasks

Splaktar mentioned this pull request Jun 24, 2019

feat(top-app-bar): migrate toolbar to top-app-bar angular/components#13964

Closed

14 tasks


		# Top App Bar

		MDC Top App Bar acts as a container for items such as


		let click;

		td.when(mockAdapter.registerNavigationIconInteractionHandler('click', td.matchers.isA(Function)))

feat(top-app-bar): Add top app bar component #2225

feat(top-app-bar): Add top app bar component #2225

Conversation

williamernest commented Feb 10, 2018

codecov-io commented Feb 12, 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

kfranqueiro Feb 14, 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

Choose a reason for hiding this comment

kfranqueiro commented Feb 14, 2018

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

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

acdvorak left a 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

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

kfranqueiro left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 12, 2018 •

edited

Loading

kfranqueiro Feb 14, 2018 •

edited

Loading