fix(top-app-bar): foundation should be destroyed and reinitialized on variant change #519

mgr34 · 2018-12-13T22:08:22Z

this should close issue #517. hope I didn't go overboard on the unit tests. As noted in discord I am having trouble getting screenshot tests to run. I have added a new one.

I signed it

… variant changes

…nitialized on variant changes

…nitialized on variant changes (adding missing screenshot test)

codecov-io · 2018-12-13T22:12:24Z

Codecov Report

Merging #519 into rc0.8.0 will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           rc0.8.0     #519      +/-   ##
===========================================
+ Coverage    96.63%   96.64%   +0.01%     
===========================================
  Files           59       59              
  Lines         1991     1999       +8     
  Branches       239      241       +2     
===========================================
+ Hits          1924     1932       +8     
  Misses          67       67

Impacted Files	Coverage Δ
packages/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 160cf3a...4bf447a. Read the comment docs.

moog16 · 2018-12-14T19:07:50Z

please water branch with rc0.8.0

moog16

Haven't done a full review, but one comment so far.

packages/top-app-bar/index.js

…riant-change

mgr34 · 2018-12-14T22:05:09Z

please water branch with rc0.8.0

I'm afraid I am not quite sure what you mean by water. Yesterday during my work on the PR I fell behind one commit. I merged that in this afternoon: 0af79bd

test/screenshot/top-app-bar/prominentToShortCollapsed.js

moog16 · 2018-12-14T22:45:37Z

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

@@ -204,6 +204,111 @@ test('#enableRippleOnElement throws error if a native element', () => {
  );
 });

+test('when changes from short to fixed the foundation changes', () => {


wow great test coverage!

moog16 · 2018-12-14T22:47:25Z

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

+  assert.exists(wrapper.instance().foundation_);
+});
+
+test('when changes from short to shortCollpased the foundation changes', () => {


Shouldn't the foundation not change in this case?

From what I understand it should reinitialize. When init() is called on MDCShortTopAppBarFoundation it registers a resize handler if initialized as a short variant. If it begins life collapsed it would not be necessary to register the resize handler. [0] Alternatively the resize handler could simply be deregistered. But I think this would work just as well?

[0] https://github.com/material-components/material-components-web/blob/a487783c74a42104a27ed6de42bad9d6d29a0a63/packages/mdc-top-app-bar/short/foundation.js#L44-L56

ah right - this is because you would want the short collapsed to immediately collapse. Good call. Thanks!

moog16 · 2018-12-14T22:49:07Z

Rereviewed your changes!

Watering a branch is making it up to date with rc0.8.0 (the base branch)

moog16 · 2018-12-17T06:05:05Z

packages/top-app-bar/index.js

@@ -69,10 +69,25 @@ export default class TopAppBar extends React.Component {
    this.foundation_.destroy();
  }

+  componentDidUpdate(prevProps) {
+    const foundationChanged = ['short', 'shortCollapsed', 'fixed']
+      .some((variant) => this.props[variant] !== prevProps[variant] );


formatting issue, but for the sake of time I'm going to ignore this one and fix in a following pr

moog16

Changes look good!

moog16 · 2018-12-17T06:24:08Z

#537

@mgr34 please update your golden json file like in this commit

moog16 · 2018-12-17T06:24:51Z

Also looks like the CLA isn't signed. Could you please sign the PR please.

mgr34 · 2018-12-17T14:58:16Z

@moog16 thanks for the review. I have merged bb7765a to update golden.json.

I am afraid the CLA thing is a bit perplexing. I know it had been signed. But I see this morning that it was not verifying. However, after pushing the merge it seems to now be verifying the signature again.

moog16 · 2018-12-17T17:19:33Z

Ya looks like its back to being green! Thanks :) I will merge it

moog16 · 2018-12-17T17:20:08Z

@mgr34 please resolve conflicts

moog16 · 2018-12-17T19:10:05Z

updated tests #539

moog16 · 2018-12-17T19:17:54Z

Tests passing in #539 and CLA signed #519 (comment)

… variant change (#519)

mgr34 added 3 commits December 13, 2018 14:28

fix(top-app-bar): foundation should be destoryed and reinitialized on…

90d9943

… variant changes

fix(top-app-bar): testing that foundation should be destoryed and rei…

ea64e2c

…nitialized on variant changes

fix(top-app-bar): testing that foundation should be destoryed and rei…

6fe6c52

…nitialized on variant changes (adding missing screenshot test)

This was referenced Dec 14, 2018

feat(top-app-bar): is missing dense variant #518

Closed

refactor top-app-bar to make use of a type property? #522

Closed

moog16 suggested changes Dec 14, 2018

View reviewed changes

packages/top-app-bar/index.js Outdated Show resolved Hide resolved

mgr34 added 2 commits December 14, 2018 14:53

Merge remote-tracking branch 'origin/rc0.8.0' into fix/top-app-bar/va…

0af79bd

…riant-change

fix(top-app-bar): dropped filter in favor of some

405a109

moog16 suggested changes Dec 14, 2018

View reviewed changes

fix(top-app-bar): simplifying shouldComponentUpdate function

1cebcef

moog16 reviewed Dec 17, 2018

View reviewed changes

moog16 approved these changes Dec 17, 2018

View reviewed changes

WIP: add golden

bb7765a

moog16 mentioned this pull request Dec 17, 2018

TESTS DO NOT MERGE: Mgr34 fix/top app bar/variant change #537

Closed

Merge branch 'rc0.8.0' into fix/top-app-bar/variant-change

4bf447a

moog16 mentioned this pull request Dec 17, 2018

DO NOT MERGE TESTS: Mgr34 fix/top app bar/variant change #539

Closed

moog16 merged commit 689b792 into material-components:rc0.8.0 Dec 17, 2018

moog16 pushed a commit that referenced this pull request Dec 28, 2018

fix(top-app-bar): foundation should be destroyed and reinitialized on…

66a1619

… variant change (#519)

moog16 pushed a commit that referenced this pull request Dec 28, 2018

fix(top-app-bar): foundation should be destroyed and reinitialized on…

58cfc6f

… variant change (#519)

fix(top-app-bar): foundation should be destroyed and reinitialized on variant change #519

fix(top-app-bar): foundation should be destroyed and reinitialized on variant change #519

Uh oh!

Conversation

mgr34 commented Dec 13, 2018

Uh oh!

codecov-io commented Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

moog16 commented Dec 14, 2018

Uh oh!

moog16 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mgr34 commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

moog16 Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

moog16 Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

mgr34 Dec 15, 2018

Choose a reason for hiding this comment

Uh oh!

moog16 Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

moog16 commented Dec 14, 2018

Uh oh!

moog16 Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

moog16 left a comment

Choose a reason for hiding this comment

Uh oh!

moog16 commented Dec 17, 2018

Uh oh!

moog16 commented Dec 17, 2018

Uh oh!

mgr34 commented Dec 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moog16 commented Dec 17, 2018

Uh oh!

moog16 commented Dec 17, 2018

Uh oh!

moog16 commented Dec 17, 2018

Uh oh!

moog16 commented Dec 17, 2018

Uh oh!

Uh oh!

codecov-io commented Dec 13, 2018 •

edited

Loading

mgr34 commented Dec 14, 2018 •

edited

Loading

mgr34 commented Dec 17, 2018 •

edited

Loading