feat(top app bar): Add short top app bar always collapsed feature #2327

williamernest · 2018-03-01T16:07:40Z

Resolves #2224

codecov-io · 2018-03-01T16:19:46Z

Codecov Report

Merging #2327 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2327      +/-   ##
==========================================
+ Coverage   98.85%   98.85%   +<.01%     
==========================================
  Files         100      100              
  Lines        4115     4118       +3     
  Branches      526      527       +1     
==========================================
+ Hits         4068     4071       +3     
  Misses         47       47

Impacted Files	Coverage Δ
packages/mdc-top-app-bar/foundation.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 0ba7d10...3d2d231. Read the comment docs.

kfranqueiro · 2018-03-01T18:14:44Z

demos/top-app-bar.html

@@ -160,16 +160,20 @@
    <div class="demo-controls-container">
      <h3>Demo Controls</h3>
      <div>
-        <input type="checkbox" id="rtl-checkbox"/>
-        <label for="rtl-checkbox">RTL</label>
+          <input type="checkbox" id="rtl-checkbox"/>


For some reason these are now double-nested?

kfranqueiro · 2018-03-01T20:51:25Z

demos/top-app-bar.html

-        <input type="checkbox" id="no-action-item-checkbox"/>
-        <label for="no-action-item-checkbox">No Action Item</label>
+          <input type="checkbox" id="no-action-item-checkbox"/>
+          <label for="no-action-item-checkbox">No Action Item</label>


I'd move this one up before Short to keep the Short-related things together

kfranqueiro · 2018-03-01T20:51:53Z

demos/top-app-bar.html

@@ -188,6 +192,7 @@ <h3>Demo Controls</h3>
        var shortCheckbox = document.getElementById('short-checkbox');
        var noActionItemCheckbox = document.getElementById('no-action-item-checkbox');
        var rtlCheckbox = document.getElementById('rtl-checkbox');
+        var alwaysClosedCheckbox = document.getElementById('always-closed-checkbox');


Nit: reorder these to match DOM?

kfranqueiro · 2018-03-01T20:53:45Z

demos/top-app-bar.html

          }
+
+          alwaysClosedCheckbox.disabled = !this.checked;


I'd suggest adding a disabled label style like in our text field demo page, to make the disabled state more obvious.

kfranqueiro · 2018-03-01T20:54:31Z

demos/top-app-bar.html

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


Same comment as last time, move the ternary to within square brackets like what was done for shortCheckbox in the previous PR

kfranqueiro · 2018-03-01T20:55:10Z

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

@@ -68,6 +68,22 @@ Short top app bars should only be used with one action item:
 </header>
 ```

+Short top app bars can be configured to stay collapsed by applying the `mdc-top-app-bar--short-collapsed` before instantiating the component : 


I might re-word "stay" to "always appear"

kfranqueiro · 2018-03-01T21:02:04Z

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

  }

  /**
-   * Scroll handler for applying/removing the closed modifier class
+   * Scroll handler for applying/removing the collapsed modifier class


Hey I missed this one last time didn't I 👀

kfranqueiro · 2018-03-01T21:08:14Z

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

+    },
+  };
+  util.applyPassive(mockWindow, true);
+  assert.deepEqual(util.applyPassive(mockWindow, false), {passive: true});


This is kind of tangential to this PR anyway, but is this really testing what we need it to test? I think this would pass whether we called it with force=true or false. (Also force=true mainly exists for tests to begin with.)

I added this to improve branch coverage to 100%. However, after referencing mdc-ripple I'll add applyPassive to the deregister adapter method so that it gets his twice in a test.

kfranqueiro

One more comment otherwise LGTM

kfranqueiro · 2018-03-02T15:51:19Z

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

@@ -89,7 +89,7 @@ class MDCTopAppBar extends MDCComponent {
          this.emit(strings.NAVIGATION_EVENT, {});
        },
        registerScrollHandler: (handler) => window.addEventListener('scroll', handler, util.applyPassive()),
-        deregisterScrollHandler: (handler) => window.removeEventListener('scroll', handler),


applyPassive should not be necessary here, actually (only useCapture matters to removeEventListener for matching): https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/removeEventListener#Matching_event_listeners_for_removal

@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)

williamernest added 3 commits February 28, 2018 14:52

feat(top-app-bar): Add always collapsed feature to short top app bar

667361e

feat(top-app-bar): Update tests/demo

8455cfb

feat(top-app-bar): Update Readme

8a1a961

williamernest assigned kfranqueiro Mar 1, 2018

feat(top-app-bar): Update demo names

99b066b

kfranqueiro suggested changes Mar 1, 2018

View reviewed changes

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

47317a4

williamernest changed the title ~~feat(top app bar): Add short top app bar always closed feature~~ feat(top app bar): Add short top app bar always collapsed feature Mar 1, 2018

feat(top-app-bar): Update readme.

7593d0b

kfranqueiro approved these changes Mar 2, 2018

View reviewed changes

williamernest and others added 2 commits March 2, 2018 12:57

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

12528ab

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

3d2d231

williamernest merged commit bc17291 into master Mar 2, 2018

williamernest deleted the feat/top-app-bar/short-always-closed branch March 2, 2018 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(top app bar): Add short top app bar always collapsed feature #2327

feat(top app bar): Add short top app bar always collapsed feature #2327

williamernest commented Mar 1, 2018

codecov-io commented Mar 1, 2018 •

edited

Loading

kfranqueiro Mar 1, 2018

kfranqueiro Mar 1, 2018

kfranqueiro Mar 1, 2018

kfranqueiro Mar 1, 2018

kfranqueiro Mar 1, 2018

kfranqueiro Mar 1, 2018

kfranqueiro Mar 1, 2018

kfranqueiro Mar 1, 2018

williamernest Mar 1, 2018

kfranqueiro left a comment

kfranqueiro Mar 2, 2018

feat(top app bar): Add short top app bar always collapsed feature #2327

feat(top app bar): Add short top app bar always collapsed feature #2327

Conversation

williamernest commented Mar 1, 2018

codecov-io commented Mar 1, 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

kfranqueiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 1, 2018 •

edited

Loading