Closed Bug 1140481 Opened 5 years ago Closed 5 years ago

Use the StoreMixin in some of the Loop call views


(Hello (Loop) :: Client, defect, P2)



(firefox39 fixed)

39.2 - 23 Mar
Tracking Status
firefox39 --- fixed
Blocking Flags:
backlog tech-debt


(Reporter: standard8, Assigned: standard8)



(Whiteboard: [tech-debt])


(2 files, 1 obsolete file)

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)
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)
Attached patch diff -w versionSplinter Review
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 ={});
> +
> +      store = new testStore(dispatcher);
> +
> +{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+
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:
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:
Flags: needinfo?(standard8)
You need to log in before you can comment on or make changes to this bug.