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

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Mark Banner (:standard8)]

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

Comment 2

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

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.

Comment 3

[Mark Banner (:standard8)]

Checked in with comments addressed:

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

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c89fd774b81

Comment 5

[u279076]

[Tracking Requested - why for this release]: nominated since this is tracked to block Loop 35.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/13c96ad43e4d

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

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