I5011 improve collection success msg appearance #5113

rebmullin · 2018-05-25T20:33:44Z

fixes mozilla/addons#11765

video
https://monosnap.com/image/52gnX55LZiRElcneQSKGCGWiJAHHwN.png

I feel like we should add some transition effect here too...

bobsilverberg · 2018-05-26T13:11:30Z

I feel like we should add some transition effect here too...

I agree, we should use a css transition to make it fade out, rather than just disappear after 5 seconds.

codecov-io · 2018-05-29T17:42:49Z

Codecov Report

Merging #5113 into master will increase coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    mozilla/addons-frontend#5113      +/-   ##
==========================================
+ Coverage    96.7%   96.84%   +0.14%     
==========================================
  Files         214      215       +1     
  Lines        5062     5172     +110     
  Branches     1012     1022      +10     
==========================================
+ Hits         4895     5009     +114     
+ Misses        149      145       -4     
  Partials       18       18

Impacted Files	Coverage Δ
src/amo/components/CollectionManager/index.js	`100% <100%> (ø)`	⬆️
src/core/api/index.js	`100% <0%> (ø)`	⬆️
src/amo/api/collections.js	`100% <0%> (ø)`	⬆️
src/amo/api/users.js	`100% <0%> (ø)`	⬆️
src/amo/sagas/collections.js	`100% <0%> (ø)`	⬆️
src/amo/components/UserProfileEdit/index.js	`100% <0%> (ø)`	⬆️
src/amo/sagas/users.js	`100% <0%> (ø)`	⬆️
src/amo/components/AddonsCard/index.js	`100% <0%> (ø)`	⬆️
src/amo/reducers/users.js	`100% <0%> (ø)`	⬆️
src/amo/components/Collection/index.js	`100% <0%> (ø)`	⬆️
... and 4 more

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 681e839...da7cdf2. Read the comment docs.

rebmullin · 2018-05-29T17:51:03Z

hey @bobsilverberg - I've updated to use a transition. Please let me know what you think about this and some reallocation of the spacing (I've adjusted spacing here a bit b/c IMO it still looked a little odd when the space changes):

https://monosnap.com/image/mdmViit8JIIB6UPrBLlGP0s7G9dTNf.png

also - I noticed the library has updated to v2:
https://github.com/reactjs/react-transition-group. Should I create a new issue to do an upgrade? It looks like its used in the disco pane too..

bobsilverberg

Nice work, thanks @rebmullin. I didn't know about ReactCSSTransitionGroup. I assumed we'd just use css transitions manually, but if it makes it easier that's cool.

Regarding the version, that's odd because Greenkeeper usually keeps us up to date on out-of-date dependencies. Are you sure the new version is the latest stable version?

I find the video difficult to view. For ease of verification could you also include just a screenshot of the message when it is on the screen? We should get @pwalm to sign off on the final positioning of that.

I'm not sure what the standard are for transitions, but I imagined something a bit more gradual. I.e., the notice would appear, and remain static for a couple of seconds, and then fade away gradually over the remaining 3 seconds. Again, maybe @pwalm has an opinion on this.

The code looks good, and I just have one question about the test, so this is an r+wc, but we should wait to hear back from @pwalm before merging it.

bobsilverberg · 2018-05-29T18:35:53Z

tests/unit/amo/components/TestCollectionManager.js

@@ -61,6 +64,10 @@ describe(__filename, () => {
    });
  });

+  afterEach(() => {
+    clock.restore();


If we're only using the clock in one test, is there a reason we have to do this in afterEach?

altho - i don't think it would hurt having this here... I am just thinking if another tests is added with a clock..

I also see clock some other files where it appears only to be used 1 time and has this similar set up...

bobsilverberg · 2018-05-29T18:36:38Z

tests/unit/amo/components/TestCollectionManager.js

+
+    state = root.state();
+    expect(state.addonAddedStatus).toEqual(null);
+    root.setProps({ hasAddonBeenAdded: true });


I'm not sure why you're setting the state here again, and I would expect odd results because of this. Was this just a line of code left in by accident?

that's what I thought too but it seems that I need to get the updated state (when I do it this way it works :/)
I also had to setProps too which I thought was odd but again it worked this way. Is it possible this is happening b/c i am missing some other piece of code here?

rebmullin · 2018-05-29T20:12:10Z

I assumed we'd just use css transitions manually, but if it makes it easier that's cool.

We could but I think this library helps with transitioning when the html is removed from the DOM.

Regarding the version, that's odd because Greenkeeper usually keeps us up to date on out-of-date dependencies. Are you sure the new version is the latest stable version?

From what i can tell, v2 has been out for a while (almost a year), altho it does say v1 is still actively maintained

here's a screenshot of before and after (to get an idea of spacing adjustments):

also - here is an updated video which i * think * might show a little better:
https://monosnap.com/file/5Wi6NTTC5DPQlouBmOOZxaIzJh5QD9#

I agree the transition seems a little fast. I did look at photon which I think suggests even faster but yeh I agree.. best to see what @pwalm thinks for this scenario

willdurand · 2018-05-30T15:42:46Z

src/amo/components/CollectionManager/index.js

@@ -89,6 +93,14 @@ export class CollectionManagerBase extends React.Component<Props, State> {
      this.setState({
        addonAddedStatus: props.hasAddonBeenAdded ?
          ADDON_ADDED_STATUS_SUCCESS : null,
+      }, () => {


We decided not to use the setState second argument in https://github.com/mozilla/addons-frontend/issues/3077.

oh? ok.. let me try another way...

willdurand · 2018-05-30T15:43:39Z

src/amo/components/CollectionManager/index.js

@@ -89,6 +93,14 @@ export class CollectionManagerBase extends React.Component<Props, State> {
      this.setState({
        addonAddedStatus: props.hasAddonBeenAdded ?
          ADDON_ADDED_STATUS_SUCCESS : null,
+      }, () => {
+        if (props.hasAddonBeenAdded) {
+          setTimeout(() => {


I think we could inject setTimeout as a default prop and pass a function that executes directly in the test cases.

hey @willdurand - do you mean something like this?

static defaultProps = { setTimeout: () => {}, };

I guess I am not totally clear of the benefit of this..? let me know if i am missing something or if something like this is ok:

componentWillReceiveProps(props: Props) { .... if (hasAddonBeenAdded !== hasAddonBeenAddedNew) { this.setState({ addonAddedStatus: props.hasAddonBeenAdded ? ADDON_ADDED_STATUS_SUCCESS : null, }); } // If the message status has been set, we'll reset the message status after 5 seconds if (hasAddonBeenAddedNew && hasAddonBeenAddedNew !== hasAddonBeenAdded) { setTimeout(this.resetMessageStatus, MESSAGE_RESET_TIME); } }

I mean having a prop called setTimeout that is configured with setTimeout :D

In the tests, we can pass (func) => func() to that prop. That way no need to deal with ticks in the tests. The benefit is that it is simpler.

willdurand · 2018-05-30T15:45:19Z

Regarding the version, that's odd because Greenkeeper usually keeps us up to date on out-of-date dependencies. Are you sure the new version is the latest stable version?

I think we never upgraded by fear of breaking things, but we should...

bobsilverberg · 2018-05-30T15:50:30Z

I think we never upgraded by fear of breaking things, but we should...

Thanks @willdurand. @rebmullin could you open a separate issue for that?

pwalm · 2018-05-30T16:26:39Z

Looks beautiful! Ship it!

rebmullin · 2018-05-30T18:32:22Z

..@rebmullin could you open a separate issue for that?

sure @bobsilverberg. But how would we go about updating this if Greenkeeper didn't? is it just a manually change?

rebmullin · 2018-05-30T21:03:40Z

In the tests, we can pass (func) => func() to that prop. That way no need to deal with ticks in the tests..

hey @willdurand, I still had to use ticks so I may be missing something. Can you please take a look at my changes when you can? thanks

willdurand · 2018-05-30T21:46:21Z

hey @willdurand, I still had to use ticks so I may be missing something. Can you please take a look at my changes when you can? thanks

Sorry, here is what I had in mind. Note that we may have to rename the prop from setTimeout to _setTimeout (and to write _setTimeout: setTimeout in the defaultProps). The idea is to fully control the setTimeout without having to deal with clock/tick. We often do this for things like window or config for example.

diff --git a/src/amo/components/CollectionManager/index.js b/src/amo/components/CollectionManager/index.js
index 554d44fe3..31cf9c8e7 100644
--- a/src/amo/components/CollectionManager/index.js
+++ b/src/amo/components/CollectionManager/index.js
@@ -78,7 +78,7 @@ type State = {|
 
 export class CollectionManagerBase extends React.Component<Props, State> {
   static defaultProps = {
-    setTimeout: (fn) => setTimeout(fn, MESSAGE_RESET_TIME),
+    setTimeout,
   };
 
   constructor(props: Props) {
@@ -104,7 +104,7 @@ export class CollectionManagerBase extends React.Component<Props, State> {
     }
 
     if (hasAddonBeenAddedNew && hasAddonBeenAddedNew !== hasAddonBeenAdded) {
-      this.props.setTimeout(this.resetMessageStatus);
+      this.props.setTimeout(this.resetMessageStatus, MESSAGE_RESET_TIME);
     }
   }
 
diff --git a/tests/unit/amo/components/TestCollectionManager.js b/tests/unit/amo/components/TestCollectionManager.js
index ff08d05b1..b8a2d7730 100644
--- a/tests/unit/amo/components/TestCollectionManager.js
+++ b/tests/unit/amo/components/TestCollectionManager.js
@@ -44,7 +44,6 @@ const simulateAutoSearchCallback = (props = {}) => {
 };
 
 describe(__filename, () => {
-  let clock;
   let fakeRouter;
   let store;
   const apiHost = config.get('apiHost');
@@ -55,7 +54,6 @@ describe(__filename, () => {
   beforeEach(() => {
     fakeRouter = createFakeRouter();
     store = dispatchClientMetadata().store;
-    clock = sinon.useFakeTimers();
     dispatchSignInActions({
       lang,
       store,
@@ -64,10 +62,6 @@ describe(__filename, () => {
     });
   });
 
-  afterEach(() => {
-    clock.restore();
-  });
-
   const getProps = ({
     collection = createInternalCollection({
       detail: createFakeCollectionDetail(),
@@ -707,7 +701,8 @@ describe(__filename, () => {
   });
 
   it('displays a notification for 5 seconds after an add-on has been added', () => {
-    const root = render({});
+    const setTimeoutSpy = sinon.stub();
+    const root = render({ setTimeout: setTimeoutSpy });
 
     expect(root.find(Notice)).toHaveLength(0);
 
@@ -715,18 +710,15 @@ describe(__filename, () => {
 
     expect(root.find(Notice)).toHaveLength(1);
     expect(root.find(Notice).children()).toHaveText('Added to collection');
+    expect(root).toHaveState('addonAddedStatus', ADDON_ADDED_STATUS_SUCCESS);
+    sinon.assert.calledWith(setTimeoutSpy, root.instance().resetMessageStatus, 5000);
 
-    const state = root.state();
-    expect(state.addonAddedStatus).toEqual(ADDON_ADDED_STATUS_SUCCESS);
-
-    const reset = () => {
-      root.setState({ addonAddedStatus: null });
-    };
-
-    // After 5 seconds the Notice will go away.
-    root.setProps(setTimeout(reset));
+    // Simulate the setTimeout behavior.
+    root.instance().resetMessageStatus();
+    // See: https://github.com/airbnb/enzyme/blob/master/docs/guides/migration-from-2-to-3.md#for-mount-updates-are-sometimes-required-when-they-werent-before
+    root.update();
 
-    clock.tick(6000);
+    expect(root).toHaveState('addonAddedStatus', null);
     expect(root.find(Notice)).toHaveLength(0);
   });

Does that make sense?

rebmullin · 2018-05-30T23:24:29Z

@willdurand, thanks for helping with this : ) I think it makes sense - basically i think your point is you can call that same function from the test thus avoiding the clock stuff. I also am seeing that I need to dig into advanced testing a bit more :0
I pretty much followed what you noted above but with a small tweak to get it fully working. let me know how it looks now : )

willdurand · 2018-05-31T08:22:09Z

src/amo/components/CollectionManager/index.js

@@ -72,25 +79,40 @@ type State = {|
 |};

 export class CollectionManagerBase extends React.Component<Props, State> {
+  static defaultProps = {
+    setTimeout,
+    _window: typeof window !== 'undefined' ? window : {},


The alternative would be to bind the setTimeout function to window:

setTimeout: typeof window !== 'undefined' ? window.setTimeout.bind(window) : () => {},

willdurand · 2018-05-31T08:23:56Z

src/amo/components/CollectionManager/index.js

-          </Notice>
-        )}
+
+        <ReactCSSTransitionGroup


I forgot how to use this component.. do we have to keep it all the time in the UI tree? (or can we put it in the condition where Notice is).

yes - it has to always be in the DOM to work

willdurand · 2018-05-31T08:25:06Z

src/amo/components/CollectionManager/index.js

+        <ReactCSSTransitionGroup
+          className="NoticePlaceholder"
+          component="div"
+          transitionName="overlay"


Is it a predefined name or custom?

No i meant to change that... thanks for catching

willdurand · 2018-05-31T08:25:22Z

tests/unit/amo/components/TestCollectionManager.js


    expect(root.find(Notice)).toHaveLength(0);

    root.setProps({ hasAddonBeenAdded: true });

    expect(root.find(Notice)).toHaveLength(1);
    expect(root.find(Notice).children()).toHaveText('Added to collection');
+
+    expect(root).toHaveState('addonAddedStatus', ADDON_ADDED_STATUS_SUCCESS);
+    sinon.assert.calledWith(setTimeoutSpy, root.instance().resetMessageStatus, 5000);


I guess here we can use MESSAGE_RESET_TIME

willdurand · 2018-05-31T08:27:02Z

tests/unit/amo/components/TestCollectionManager.js

+
+    // Simulate the setTimeout behavior.
+    root.instance().resetMessageStatus();
+    // See: https://github.com/airbnb/enzyme/blob/master/docs/guides/migration-from-2-to-3.md#for-mount-updates-are-sometimes-required-when-they-werent-before


Maybe we can use this link with a git tag in the url: https://github.com/airbnb/enzyme/blob/enzyme%403.3.0/docs/guides/migration-from-2-to-3.md#for-mount-updates-are-sometimes-required-when-they-werent-before. That way it should always be valid.

rebmullin · 2018-05-31T19:29:56Z

@willdurand, thanks for the feedback. how's it look now?

willdurand

👍

I5011 improve collection success msg

f433197

rebmullin changed the title ~~I5011 improve collection success msg~~ I5011 improve collection success msg appearance May 25, 2018

bobsilverberg self-assigned this May 26, 2018

adding transition effect

97e31ee

bobsilverberg self-requested a review May 29, 2018 18:31

bobsilverberg approved these changes May 29, 2018

View reviewed changes

willdurand reviewed May 30, 2018

View reviewed changes

updating some per request

8c40cf1

rebmullin added 2 commits May 30, 2018 19:17

updating per comments

d294993

removing clock stuff

e17dbd5

willdurand reviewed May 31, 2018

View reviewed changes

updating per comments

da7cdf2

rebmullin requested a review from willdurand May 31, 2018 19:30

willdurand approved these changes May 31, 2018

View reviewed changes

rebmullin merged commit 3e5b031 into mozilla:master May 31, 2018

rebmullin deleted the I5011-improve-collection-success-msg branch August 1, 2018 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

I5011 improve collection success msg appearance #5113

I5011 improve collection success msg appearance #5113

rebmullin commented May 25, 2018

bobsilverberg commented May 26, 2018

codecov-io commented May 29, 2018 •

edited

Loading

rebmullin commented May 29, 2018

bobsilverberg left a comment

bobsilverberg May 29, 2018

rebmullin May 29, 2018

rebmullin May 29, 2018

bobsilverberg May 29, 2018

rebmullin May 29, 2018

rebmullin commented May 29, 2018

willdurand May 30, 2018

rebmullin May 30, 2018

willdurand May 30, 2018

rebmullin May 30, 2018

willdurand May 30, 2018 •

edited

Loading

willdurand commented May 30, 2018

bobsilverberg commented May 30, 2018

pwalm commented May 30, 2018

rebmullin commented May 30, 2018

rebmullin commented May 30, 2018

willdurand commented May 30, 2018

rebmullin commented May 30, 2018

willdurand May 31, 2018

willdurand May 31, 2018

rebmullin May 31, 2018

willdurand May 31, 2018

rebmullin May 31, 2018

willdurand May 31, 2018

willdurand May 31, 2018

rebmullin commented May 31, 2018

willdurand left a comment

I5011 improve collection success msg appearance #5113

I5011 improve collection success msg appearance #5113

Conversation

rebmullin commented May 25, 2018

bobsilverberg commented May 26, 2018

codecov-io commented May 29, 2018 • edited Loading

Codecov Report

rebmullin commented May 29, 2018

bobsilverberg 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

rebmullin commented May 29, 2018

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

willdurand May 30, 2018 • edited Loading

Choose a reason for hiding this comment

willdurand commented May 30, 2018

bobsilverberg commented May 30, 2018

pwalm commented May 30, 2018

rebmullin commented May 30, 2018

rebmullin commented May 30, 2018

willdurand commented May 30, 2018

rebmullin commented May 30, 2018

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

rebmullin commented May 31, 2018

willdurand left a comment

Choose a reason for hiding this comment

codecov-io commented May 29, 2018 •

edited

Loading

willdurand May 30, 2018 •

edited

Loading