Closed Bug 1106852 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Points:
5

Tracking

(Not tracked)

RESOLVED FIXED
mozilla38
Iteration:
38.1 - 26 Jan
Blocking Flags:
backlog tech-debt

People

(Reporter: standard8, Unassigned)

Details

(Whiteboard: [tech-debt])

Attachments

(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+
https://hg.mozilla.org/integration/fx-team/rev/ef61f646f361
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
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.