Closed Bug 1106852 Opened 6 years ago Closed 6 years ago

Create a store mixin for components to listen to stores and updating state


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



(Not tracked)

38.1 - 26 Jan
Blocking Flags:
backlog tech-debt


(Reporter: standard8, Unassigned)


(Whiteboard: [tech-debt])


(1 file, 2 obsolete files)

We've got various bits of repeated code for listening to stores. We should consider making a StoreMixin to make it easier to update state based on stores.
backlog: --- → Fx37?
Attached patch Store mixin (WiP) (obsolete) — Splinter Review
This is an experimental but working approach; thoughts?
Attachment #8532590 - Flags: feedback?(standard8)
Attached patch Store mixin (WiP). Patch v2 (obsolete) — Splinter Review
Patch now migrates feedback views to use the new RoomStore mixin.
Attachment #8532590 - Attachment is obsolete: true
Attachment #8532590 - Flags: feedback?(standard8)
Attachment #8532625 - Flags: feedback?(standard8)
Comment on attachment 8532625 [details] [diff] [review]
Store mixin (WiP). Patch v2

Review of attachment 8532625 [details] [diff] [review]:

I've not looked in too much detail, but the general idea seems fine to me.
Attachment #8532625 - Flags: feedback?(standard8) → feedback+
backlog: Fx37? → Fx38?
backlog: Fx38? → tech-debt
Priority: -- → P3
I reduced the scope of the previous patch to ease its review, and rebased it on top of latest trunk. Once this one is eventually r+, I'll migrate other components to use this new mixin iteratively.
Attachment #8532625 - Attachment is obsolete: true
Attachment #8551862 - Flags: review?(standard8)
Comment on attachment 8551862 [details] [diff] [review]
Introducing StoreMixin for Loop.

Review of attachment 8551862 [details] [diff] [review]:

This looks good, especially the fact that we simplify the props.

It might just be worth passing through the test files and see if you don't need to pass in a feedback store to components now - adding the register for the mixin implies something was using it, so you should be able to not pass it in a few instances hopefully.

r=Standard8 with the comments fixed.

::: browser/components/loop/content/js/roomViews.js
@@ +127,5 @@
>      render: function() {
>        var cx = React.addons.classSet;
>        return (
> +        React.createElement("div", {className: "room-invitation-overlay"},

Please check the generation of this file - your version seems to have no trailing spaces, but my jsx is adding them back

::: browser/components/loop/content/shared/js/feedbackViews.jsx
@@ +225,2 @@
>        onAfterFeedbackReceived: React.PropTypes.func,
> +      // Used by the UI showcase.s

nit: unnecessary extra s

::: browser/components/loop/test/shared/feedbackViews_test.js
@@ +29,3 @@
>      comp = TestUtils.renderIntoDocument(
>        React.createElement(sharedViews.FeedbackView, {
>          feedbackStore: feedbackStore

I believe its now not necessary to pass the store in here.
Attachment #8551862 - Flags: review?(standard8) → review+
Iteration: --- → 38.1 - 26 Jan
Points: --- → 5
Target Milestone: --- → mozilla38
Adding leave-open because other components will need to migrate to using this new mixin as well. We'll mark this as fixed once everything is properly migrated.
Keywords: leave-open
Rank: 35
Flags: firefox-backlog+
We landed this in gecko 38, and have been rolling it out in a few more places in other bugs. Given we've got bug 1082729 on a similar issue as well, I think we'll just close this one and have that cover the remaining work.
Assignee: nobody → nperriault
Closed: 6 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.