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

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Store mixin (WiP)

This is an experimental but working approach; thoughts?

Comment 2

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Store mixin (WiP). Patch v2

Patch now migrates feedback views to use the new RoomStore mixin.

Comment 3

[Mark Banner (: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.

Comment 4

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Introducing StoreMixin for Loop.

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.

Comment 5

[Mark Banner (: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.

Comment 6

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

https://hg.mozilla.org/integration/fx-team/rev/ef61f646f361

Comment 7

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef61f646f361

Comment 9

[Mark Banner (:standard8)]

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.