Use the StoreMixin in some of the Loop call views

RESOLVED FIXED in Firefox 39

Status

defect
P2
normal
Rank:
23
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
mozilla39
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [tech-debt])

Attachments

(2 attachments, 1 obsolete attachment)

As part of the work for bug 1088672, I want to tidy the call views up a bit first and use the store mixin to simplify the props we're passing and the duplicated code.
This uses the StoreMixin in a few more places.

It also fixes a bug in the StoreMixin where it was stopping listening on unmount for all views attached to the store rather than just stopping for the view being unmounted.
Attachment #8574035 - Flags: review?(dmose)
Status: NEW → ASSIGNED
Flags: qe-verify-
Flags: firefox-backlog+
Comment on attachment 8574035 [details] [diff] [review]
Use the StoreMixin in some of the Loop conversation views.

Review of attachment 8574035 [details] [diff] [review]:
-----------------------------------------------------------------

r=dmose, with the one added unit test.

::: browser/components/loop/content/shared/js/store.js
@@ +135,5 @@
>            this.setState(this.getStoreState());
>          }, this);
>        },
>        componentWillUnmount: function() {
> +        this.getStore().off("change", null, this);

This would appear to fix a bug, so I'd suggest a unit test here.
Attachment #8574035 - Flags: review?(dmose) → review+
I had to write a new test section (and indent the existing one) to be able to test the store mixin, hence re-requesting review.
Attachment #8574035 - Attachment is obsolete: true
Attachment #8575502 - Flags: review?(dmose)
Comment on attachment 8575505 [details] [diff] [review]
diff -w version

Review of attachment 8575505 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with one minor tweak; thanks!  r=dmose

::: browser/components/loop/test/shared/store_test.js
@@ +163,5 @@
> +      testStore = loop.store.createStore({});
> +
> +      store = new testStore(dispatcher);
> +
> +      loop.store.StoreMixin.register({store: store});

There a lot of similarly named things here (e.g. it's not obvious on first reading how testStore and store are different).   I think part of the problem is the API itself hard-to-approach, but no need to fix that here.  If you could at least rename things here a bit more explicitly i think that would help test readability notably.
Attachment #8575505 - Flags: review+
Attachment #8575502 - Flags: review?(dmose) → review+
https://hg.mozilla.org/integration/fx-team/rev/35827fc86c8
backlog: --- → tech-debt
Whiteboard: [tech-debt]
Target Milestone: --- → mozilla39
I'd missed the ui-showcase, here's the bustage fix with rs=dmose over irc:

https://hg.mozilla.org/integration/fx-team/rev/c4d7c9f94f61
Per Gavin's request - just updating bugs that carried over before they were 100% resolved to the next iteration.
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
Rank: 23
Priority: -- → P2
There were Wr failures after those backouts (and more retriggers prior to when these patches first landed showed some Wr failures), so relanding these:
https://hg.mozilla.org/integration/fx-team/rev/7324d3da6d28
https://hg.mozilla.org/integration/fx-team/rev/f96045350d40
Flags: needinfo?(standard8)
You need to log in before you can comment on or make changes to this bug.