feat(top-app-bar): add default scroll behavior #2417

williamernest · 2018-03-16T00:24:37Z

Fixes: #2424
Adds the default scrolling behavior of the top app bar (scrolls off the screen when scrolling down, first thing to appear when scrolling up).

codecov-io · 2018-03-16T01:43:38Z

Codecov Report

Merging #2417 into master will decrease coverage by 0.02%.
The diff coverage is 97.67%.

@@            Coverage Diff             @@
##           master    #2417      +/-   ##
==========================================
- Coverage   98.64%   98.62%   -0.03%     
==========================================
  Files         103      104       +1     
  Lines        4143     4226      +83     
  Branches      518      530      +12     
==========================================
+ Hits         4087     4168      +81     
- Misses         56       58       +2

Impacted Files	Coverage Δ
packages/mdc-top-app-bar/short/foundation.js	`100% <ø> (ø)`	⬆️
packages/mdc-top-app-bar/constants.js	`100% <100%> (ø)`	⬆️
packages/mdc-top-app-bar/standard/foundation.js	`100% <100%> (ø)`
packages/mdc-top-app-bar/index.js	`100% <100%> (ø)`	⬆️
packages/mdc-top-app-bar/foundation.js	`93.1% <75%> (-6.9%)`	⬇️

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 afe5367...ce71bba. Read the comment docs.

moog16 · 2018-03-16T16:55:11Z

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

+    // Used to verify when the top app bar is completely showing or completely hidden
+    /** @private {number} */
+    this.topAppBarHeight_ = this.adapter_.getTopAppBarHeight();
+    // isDocked_ is used to indicate if the top app bar is 100% showing or hidden


Is this calculated off of topAppBarHeight? The 2 comments are very similar

Yes, the isDocked_ variable is used to determine if the top app bar is already docked. The topAppBarHeight_ and the currentAppBarScrollPosition_ is what we use to set that value.

moog16 · 2018-03-16T16:59:00Z

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

+    this.resizeThrottleId_ = -1;
+    // The timeout that's used to debounce toggling the isCurrentlyBeingResized_ variable after a resize
+    /** @private {number} */
+    this.resizeDebounceId_ = -1;

    this.navClickHandler_ = () => this.adapter_.notifyNavigationIconClicked();
    this.scrollHandler_ = () => this.shortAppBarScrollHandler_();


I think it makes more sense to rename defaultScrollHandler to scrollHandler, and scrollHandler to shortScrollHandler.

moog16 · 2018-03-16T17:37:22Z

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

+    const fullyShown = this.currentAppBarScrollPosition_ === -this.topAppBarHeight_;
+    let updateRequired = false;
+
+    if (this.isDocked_) {


Is isDocked the same as fullyShown?

In short, no.

fullyShown and fullyHidden refer to the actual location of the top app bar. isDocked_ refers to wether the previous state was fullyShown or fullyHidden. It's used to track when the top app bar reaches each terminal state (100% on/off screen) so we can ignore DOM updates if they don't change anything.

moog16 · 2018-03-16T17:38:13Z

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

  }

  /**
-   * Used to set the initial style of the short top app bar
+   * Used to set the initial style of the short top app bar.


add @Private

moog16 · 2018-03-16T17:38:47Z

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

@@ -105,16 +144,128 @@ class MDCTopAppBarFoundation extends MDCFoundation {
    const currentScroll = this.adapter_.getViewportScrollY();


add @Private to this method too

moog16 · 2018-03-16T18:12:15Z

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

+    this.resizeDebounceId_ = setTimeout((function() {
+      this.topAppBarScrollHandler_();
+      this.isCurrentlyBeingResized_ = false;
+    }).bind(this), 100);


make the 100ms a constant

moog16 · 2018-03-16T18:12:24Z

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

+      this.resizeThrottleId_ = setTimeout((function() {
+        this.resizeThrottleId_ = -1;
+        this.throttledResizeHandler_();
+      }).bind(this), 100);


make the 100ms a constant

moog16 · 2018-03-16T18:15:35Z

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

        getViewportScrollY: () => window.pageYOffset,
        getTotalActionItems: () =>
          this.root_.querySelectorAll(strings.ACTION_ITEM_SELECTOR).length,
+


remove line

moog16 · 2018-03-16T18:21:58Z

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

+});
+
+test('top app bar : resize events should set isCurrentlyBeingResized_ to true', () => {
+  // const clock = lolex.install();


moog16 · 2018-03-16T18:22:25Z

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

+  resizeHandler();
+  assert.isFalse(foundation.resizeThrottleId_ === -1);
+  resizeHandler();
+  clock.tick(101);


you can probably use that constant here that you create

moog16 · 2018-03-19T16:47:38Z

demos/top-app-bar.html


          shortCheckbox.disabled = this.checked;
+
+          // Needs to reset the toolbar height for the default scroll behavior
+          appBar.destroy();


Would this be a good case for a layout method or something to reset the scrolling? I can't think of a use case outside of the demo pages for it though.

This is only required for the demo page since we're showcasing several different toolbars on a single page.

moog16 · 2018-03-19T18:57:29Z

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

+
+    if (this.isDocked_) {
+      // If it's already docked and the user is still scrolling in the same direction, do nothing
+      if (!(fullyShown || fullyHidden)) {


instead of doing 2 variables fullyshown and fullyhidden, could you do

partiallyShowing = this.currentAppBarScrollPosition > 0 && this.currentAppBarScrollPosition < -this.topAppBarHeight_;

moog16 · 2018-03-20T16:41:01Z

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

-export {strings, cssClasses};
+/** @enum {number} */
+const numbers = {
+  MAX_TOP_APP_BAR_HEIGHT: 128,


swap these for alpha order

moog16 · 2018-03-20T16:45:02Z

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

+    let updateRequired = false;
+
+    if (this.isDocked_) {
+      // If it was previously already docked but now is partially showing, it's no longer docked


moog16 · 2018-03-20T16:51:24Z

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

+ * limitations under the License.
+ */
+
+/**


Looks like you have 2 licenses in here. You probably want to delete past this comment,

moog16 · 2018-03-20T16:52:27Z

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

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


I think you can omit the {times:1} since it is the default.

Judging by td's docs, times: 1 is not the default; the default behavior passes if it was called > 0 times.

https://github.com/testdouble/testdouble.js/blob/master/docs/6-verifying-invocations.md#times

moog16 · 2018-03-20T16:54:30Z

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

+  assert.isTrue(foundation.topAppBarHeight_ === 56);
+});
+
+test('top app bar scroll: throttledResizeHandler_ does not update topAppBarHeight_ if height is the same', () => {


This test is essentially testing the same thing as the above test. Does this cover another branch that the above does not?

No, this is testing two different branches.

moog16 · 2018-03-20T16:55:04Z

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

+  foundation.currentAppBarScrollPosition_ = -1; // Indicates 1px scrolled up
+  foundation.isUpdateRequired_ = () => true;
+  foundation.moveTopAppBar_();
+  td.verify(mockAdapter.setStyle('top', '-1px'), {times: 1});


If we decide to remove {times:1}, then remove this too.

moog16

Looks good to me

acdvorak

I still need to review the logic in packages/mdc-top-app-bar/standard/foundation.js

acdvorak · 2018-03-19T20:39:22Z

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

+/** @enum {number} */
+const numbers = {
+  MAX_TOP_APP_BAR_HEIGHT: 128,
+  DEBOUNCE_THROTTLE_RESIZE_TIME: 100,


For time values stored as integers, it's usually a good idea to include the unit of measurement in the variable name.

E.g., DEBOUNCE_THROTTLE_RESIZE_TIME_MS

acdvorak · 2018-03-19T20:44:53Z

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

+   */
+  constructor(adapter) {
+    super(adapter);
+    // Used for diffs of current scroll position vs previous scroll position


Nit: Combine these two comment lines, and add a blank line before the comment.

Ditto below.

E.g.:

super(adapter); /** * Used for diffs of current scroll position vs previous scroll position * @private {number} */ this.lastScrollPosition_ = this.adapter_.getViewportScrollY(); /** * Used to verify when the top app bar is completely showing or completely hidden * @private {number} */ this.topAppBarHeight_ = this.adapter_.getTopAppBarHeight(); ...

acdvorak · 2018-03-19T20:48:12Z

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

+    super.destroy();
+    this.adapter_.deregisterScrollHandler(this.scrollHandler_);
+    this.adapter_.deregisterResizeHandler(this.resizeHandler_);
+    this.adapter_.setStyle('top', 'auto');


It might be better to clear this inline style instead of giving it an explicit value of auto.

E.g., setStyle('top', '')

That way, it will revert to whatever was set by the baseline CSS classes, and we won't have to keep the values in sync between JS and Sass.

acdvorak · 2018-03-19T20:59:11Z

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

+  /**
+   * Function to determine if the DOM needs to update.
+   * @private
+   * @returns {boolean}


@return (no trailing s).

IntelliJ auto-generates JSDocs with a trailing s on @return for some reason, and I can't find any way to change it.

Nice catch! After some research, I found that it uses the last one you chose when you were presented with the dropdown. Start typing @retur and select @return from the dropdown and the next time it generates the JSDoc comments for you it'll use @return.

Also, JSDoc default is @returns, but it accepts @return as a synonym.

Ahh that explains it! Thanks for solving this mystery for me!

acdvorak · 2018-03-19T21:00:22Z

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

+
+  /**
+   * Function to determine if the DOM needs to update.
+   * @private


Nit: @private is usually the last annotation, after @param and @return.

acdvorak · 2018-03-27T18:46:01Z

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

+    if (this.isDocked_) {
+      // If it was previously already docked but now is partially showing, it's no longer docked.
+      if (partiallyShowing) {
+        this.isDocked_ = false;


It feels a little weird to set the value of an instance variable inside a getter method.
It's not obvious that calling isUpdateRequired_() will have side effects.

Could we structure this a little differently and have isDocked_ be a get property or something? So that it always returns the most up-to-date value (because it recomputes the value every time it's called).

Or, could we rename this method to something like checkForUpdate_(), so that it doesn't sound like a pure getter?

WDYT?

acdvorak · 2018-03-27T18:48:00Z

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

+      // If it was previously already docked but now is partially showing, it's no longer docked.
+      if (partiallyShowing) {
+        this.isDocked_ = false;
+        updateRequired = true;


Can we just return true here and remove the updateRequired variable?

acdvorak · 2018-03-28T18:49:09Z

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

+   * @return {boolean}
+   * @private
+   */
+  checkForUpdate_() {


Some of the logic is a little hard to mentally parse cuz it's so abstract.

I'm wondering if adding some local variables might make it easier to grok (and maintain).

E.g.:

checkForUpdate_() { const offscreenBoundaryTop = -this.topAppBarHeight_; const hasAnyPixelsOffscreen = this.currentAppBarScrollPosition_ < 0; const hasAnyPixelsOnscreen = this.currentAppBarScrollPosition_ > offscreenBoundaryTop; const partiallyShowing = hasAnyPixelsOffscreen && hasAnyPixelsOnscreen; // If it was previously already docked but now is partially showing, it's no longer docked. if (this.wasDocked_ && partiallyShowing) { this.wasDocked_ = false; } // If it's not previously docked and not partially showing, it just became docked. else if (!this.wasDocked_ && !partiallyShowing) { this.wasDocked_ = true; } return partiallyShowing; }

acdvorak

Just a few nits, otherwise LGTM!

acdvorak · 2018-04-05T16:03:55Z

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

@@ -47,6 +47,19 @@ class MDCTopAppBarAdapter {
   */
  hasClass(className) {}

+  /**
+   * Adds the specified attribute and value to the root Element.


Nit: Update comment to something like:

Sets the specified inline style property on the root Element to the given value.

acdvorak · 2018-04-05T16:05:13Z

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

+   * @param {string} attribute
+   * @param {string} value
+   */
+  setStyle(attribute, value) {}


Nit: Rename attribute to property

(See https://css-tricks.com/css-ruleset-terminology/)

acdvorak · 2018-04-05T16:08:22Z

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

@@ -43,11 +48,15 @@ class MDCTopAppBarFoundation extends MDCFoundation {
      hasClass: (/* className: string */) => {},
      addClass: (/* className: string */) => {},
      removeClass: (/* className: string */) => {},
+      setStyle: (/* attribute: string, value: string */) => {},


Nit: Rename attribute to property

(See https://css-tricks.com/css-ruleset-terminology/)

acdvorak · 2018-04-05T16:17:17Z

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

+     * Variable for current scroll position of the top app bar
+     * @private {number}
+     */
+    this.currentAppBarScrollPosition_ = 0;


Nit: This variable name is a little confusing to me. Can we rename it to something that conveys its use and purpose?

E.g., currentAppBarOffsetTop_

acdvorak · 2018-04-05T16:18:06Z

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

+     * The timeout that's used to throttle the resize events
+     * @private {number}
+     */
+    this.resizeThrottleId_ = -1;


Nit: Create a file-level constant for this "magic number" (ditto for all other inline -1 values below)

acdvorak · 2018-04-05T16:25:04Z

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

+      if (!this.wasDocked_) {
+        this.wasDocked_ = true;
+        return true;
+      } else {


Nitty: Combine this else with the if statement directly below it.

E.g.:

} else if (this.isDockedShowing_...) {

acdvorak · 2018-04-05T16:27:04Z

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

+   */
+  topAppBarResizeHandler_() {
+    // Throttle resize events 10 p/s
+    if (this.resizeThrottleId_ === -1) {


Nit: Magic constants

williamernest added 8 commits March 13, 2018 17:18

feat(top-app-bar): Add default scroll behavior

3f3a9f9

feat(top-app-bar): Update for safari bug

1d8113f

feat(top-app-bar): Add resize handler

3f325ec

WIP: Finish resize handling

9783de2

feat(top-app-bar): Add comments

283352d

WIP: Add fixed-adjust classes for variants

7fd0ad9

WIP: Update scroll behavior. Add comments. Add closure annotations

18c8dda

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

6a97eac

williamernest assigned acdvorak and moog16 Mar 16, 2018

moog16 suggested changes Mar 16, 2018

View reviewed changes

williamernest and others added 4 commits March 16, 2018 11:27

Merge branch 'master' into feat/top-app-bar/add-default-scroll-behavior

3fb07bc

WIP: Merge

236eb64

feat(top-app-bar): Update for comments, merge with master

4de6af2

Merge branch 'master' into feat/top-app-bar/add-default-scroll-behavior

9f51fb5

moog16 suggested changes Mar 19, 2018

View reviewed changes

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

4b9ce43

moog16 suggested changes Mar 20, 2018

View reviewed changes

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

6544484

moog16 approved these changes Mar 23, 2018

View reviewed changes

acdvorak suggested changes Mar 27, 2018

View reviewed changes

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

f7d3a46

acdvorak reviewed Mar 28, 2018

View reviewed changes

feat(top-app-bar): Update top app bar scrolling logic

e0004c0

williamernest force-pushed the feat/top-app-bar/add-default-scroll-behavior branch from f78e542 to e0004c0 Compare March 28, 2018 22:12

williamernest and others added 4 commits March 30, 2018 09:15

feat(top-app-bar): Update test. Make variable private

92a0718

feat(top-app-bar): Merge with master

fb858df

Merge branch 'master' into feat/top-app-bar/add-default-scroll-behavior

e2288ab

feat(top-app-bar): Fix linter

b6fd60c

Merge branch 'master' into feat/top-app-bar/add-default-scroll-behavior

c66923c

acdvorak approved these changes Apr 5, 2018

View reviewed changes

williamernest and others added 5 commits April 5, 2018 10:09

Merge branch 'master' into feat/top-app-bar/add-default-scroll-behavior

d2ef63d

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

8ce12e9

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

1e04327

Merge branch 'master' into feat/top-app-bar/add-default-scroll-behavior

8f5327f

Merge branch 'master' into feat/top-app-bar/add-default-scroll-behavior

ce71bba

williamernest merged commit 18be342 into master Apr 6, 2018

williamernest deleted the feat/top-app-bar/add-default-scroll-behavior branch April 6, 2018 21:54

		@@ -105,16 +144,128 @@ class MDCTopAppBarFoundation extends MDCFoundation {
		const currentScroll = this.adapter_.getViewportScrollY();

feat(top-app-bar): add default scroll behavior #2417

feat(top-app-bar): add default scroll behavior #2417

Conversation

williamernest commented Mar 16, 2018 • edited Loading

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

moog16 Mar 16, 2018 • edited by williamernest 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

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

moog16 left a comment

Choose a reason for hiding this comment

acdvorak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdvorak Mar 19, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdvorak Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

acdvorak 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

williamernest commented Mar 16, 2018 •

edited

Loading

codecov-io commented Mar 16, 2018 •

edited

Loading

moog16 Mar 16, 2018 •

edited by williamernest

Loading

acdvorak Mar 19, 2018 •

edited

Loading

acdvorak Mar 28, 2018 •

edited

Loading