Closed Bug 1094128 Opened 5 years ago Closed 5 years ago

Convert the Loop Standalone controller app view to be based on the Flux style

Categories

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

defect
Points:
2

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
36.2
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
Blocking Flags:
backlog Fx35+

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file)

For the rooms work, we're implementing using the flux style of stores and components.

To ease this work, we really need to move the existing standalone init function and controller view to be based on the flux style.

We don't need to do the rest of the flow at the current time, as that will effectively be on its own - its the triggering that we need from the flux style.

This work is based on what we do in the conversation window in desktop.
backlog: --- → Fx35+
Priority: -- → P1
Comment on attachment 8517341 [details] [diff] [review]
Convert the Loop Standalone controller app view to be based on the Flux style.

Review of attachment 8517341 [details] [diff] [review]:
-----------------------------------------------------------------

Pair reviewed over Vidyo.

Looks great, r=me with comments addressed.

::: browser/components/loop/content/shared/js/actions.js
@@ +35,4 @@
>       */
>      GetWindowData: Action.define("getWindowData", {
> +      windowId: [String, null],
> +      windowPath: [String, null]

That's a many possible combinations… How about using different action types instead?

::: browser/components/loop/standalone/content/js/standaloneAppStore.js
@@ +80,5 @@
> +
> +      if (windowPath) {
> +        // Is this a call url (the hash is a backwards-compatible url)?
> +        match = extractId(windowPath, /\#call\/(.*)/) ||
> +                extractId(windowPath, /\/c\/([\w\-]+)$/);

Nit: Could you please extract these regexps and expose them as named variables, to improve legibility a little?

@@ +112,5 @@
> +        windowType = "unsupportedDevice";
> +      } else if (!this._sdk.checkSystemRequirements()) {
> +        windowType = "unsupportedBrowser";
> +      } else if (actionData.windowPath) {
> +        [windowType, windowId] = this._extractWindowDataFromPath(actionData.windowPath);

Can we use es6 for standalone?

@@ +130,5 @@
> +      if (windowId) {
> +        this._dispatcher.dispatch(new loop.shared.actions.SetupWindowData({
> +          windowId: windowId,
> +          type: windowType
> +        }));

I think we want to use a new action here.

::: browser/components/loop/test/standalone/standaloneAppStore_test.js
@@ +147,5 @@
> +
> +      store.getWindowData(new sharedActions.GetWindowData(fakeGetWindowData));
> +
> +      expect(store.getStoreState()).eql({
> +        windowType: "unknown"

Maybe "home" would be better suited.
Attachment #8517341 - Flags: review?(nperriault) → review+
Checked in with comments addressed:

https://hg.mozilla.org/integration/fx-team/rev/9c89fd774b81
Target Milestone: --- → mozilla36
https://hg.mozilla.org/mozilla-central/rev/9c89fd774b81
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Flags: qe-verify-
Flags: in-qa-testsuite-
Flags: in-moztrap-
[Tracking Requested - why for this release]: nominated since this is tracked to block Loop 35.
Comment on attachment 8517341 [details] [diff] [review]
Convert the Loop Standalone controller app view to be based on the Flux style.

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only
Attachment #8517341 - Flags: approval-mozilla-aurora?
Attachment #8517341 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.