Closed
Bug 1140481
Opened 9 years ago
Closed 9 years ago
Use the StoreMixin in some of the Loop call views
Categories
(Hello (Loop) :: Client, defect, P2)
Hello (Loop)
Client
Tracking
(firefox39 fixed)
People
(Reporter: standard8, Assigned: standard8)
References
Details
(Whiteboard: [tech-debt])
Attachments
(2 files, 1 obsolete file)
33.69 KB,
patch
|
dmosedale
:
review+
|
Details | Diff | Splinter Review |
19.16 KB,
patch
|
dmosedale
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Updated•9 years ago
|
Status: NEW → ASSIGNED
Flags: qe-verify-
Flags: firefox-backlog+
Comment 2•9 years ago
|
||
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+
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
Comment 5•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8575502 -
Flags: review?(dmose) → review+
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/35827fc86c8
backlog: --- → tech-debt
Whiteboard: [tech-debt]
Target Milestone: --- → mozilla39
Assignee | ||
Comment 7•9 years ago
|
||
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
Comment 8•9 years ago
|
||
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
Updated•9 years ago
|
Rank: 23
Priority: -- → P2
Backed both parts out under suspicion of making the web-platform-tests-reftests intermittently fail: https://hg.mozilla.org/integration/fx-team/rev/1cf3f2f38f4e https://hg.mozilla.org/integration/fx-team/rev/0b2ed04e67ca https://treeherder.mozilla.org/logviewer.html#?job_id=2235777&repo=fx-team
Flags: needinfo?(standard8)
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)
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7324d3da6d28 https://hg.mozilla.org/mozilla-central/rev/f96045350d40
You need to log in
before you can comment on or make changes to this bug.
Description
•