1065608 - Drop the remaining backbone views for Loop (switch to react)

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Mark Banner (:standard8)]

We should finish off switching the last few (simple) views to React, this lets a drop a bunch of the router code which is currently managing the different types of view.

It also has the side-effect of making bug 1017257 far simpler by not having to load templates separately.

Comment 1

[Mark Banner (:standard8)]

Attachment: Drop the remaining backbone views for Loop (switch to react).

This switches the last few views to react, and cleans up the router code and tests.

I noticed in testing this two problems:

- Production currently doesn't work for safari due to apparent case issues in the l10n urls.
- There's no CSS / pretty view for these.

I'll make sure to get bugs filed on both of those.

Comment 2

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

Comment on attachment 8487451 [details] [diff] [review]
Drop the remaining backbone views for Loop (switch to react).

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

Looks good, r=me with comments possibly addressed and mentioned bugs filed.

(So long, Backbone views!)

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +41,5 @@
> +  var UnsupportedBrowserView = React.createClass({
> +    render: function() {
> +      var useLatestFF = mozL10n.get("use_latest_firefox", {
> +        "firefoxBrandNameLink": "<a target='_blank' href='" +
> +          "https://www.mozilla.org/firefox/'>Firefox</a>"

May I suggest we use a component as well here?

@@ +361,5 @@
>          throw new Error("WebappRouter requires a helper object");
>        }
>  
>        // Load default view
> +      this.loadReactComponent(HomeView());

You can safely use <HomeView/> here.

@@ +507,5 @@
>      /**
>       * Default entry point.
>       */
>      home: function() {
> +      this.loadReactComponent(HomeView());

<HomeView/>

@@ +512,4 @@
>      },
>  
>      unsupportedDevice: function() {
> +      this.loadReactComponent(UnsupportedDeviceView());

<UnsupportedDeviceView/>

@@ +516,4 @@
>      },
>  
>      unsupportedBrowser: function() {
> +      this.loadReactComponent(UnsupportedBrowserView());

<UnsupportedBrowserView/>

Comment 3

[Mark Banner (:standard8)]

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

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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

Comment 5

[u279076]

Does this require any manual testing?

Comment 6

[Mark Banner (:standard8)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #5)
> Does this require any manual testing?

We should probably have occasional tests that loading a call url on an unsupported browser (e.g. IE or Safari) or an unsupported device (iphone) gives appropriate messages.

Comment 7

[u279076]

(In reply to Mark Banner (:standard8) from comment #6)
> (In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #5)
> > Does this require any manual testing?
> 
> We should probably have occasional tests that loading a call url on an
> unsupported browser (e.g. IE or Safari) or an unsupported device (iphone)
> gives appropriate messages.

Is this bug just about showing an error page when loading a call URL on an unsupported browser/platform? or is there more to this that needs testing?

Comment 8

[Mark Banner (:standard8)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #7)
> (In reply to Mark Banner (:standard8) from comment #6)
> > (In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #5)
> > > Does this require any manual testing?
> > 
> > We should probably have occasional tests that loading a call url on an
> > unsupported browser (e.g. IE or Safari) or an unsupported device (iphone)
> > gives appropriate messages.
> 
> Is this bug just about showing an error page when loading a call URL on an
> unsupported browser/platform? or is there more to this that needs testing?

This bug was about adjusting how our views were displayed on a code level. However, it did make me remember that we probably don't have tests for the error pages on unsupported browsers/platforms.

Comment 9

[u279076]

Untracking this for QE verification. Testing other browsers through the Standalone UI is in scope for the Moztrap testrun so the case in comment 8 should be covered. However, we won't explicitly verify this bug as fixed.

Comment 10

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

Comment on attachment 8487451 [details] [diff] [review]
Drop the remaining backbone views for Loop (switch to react).

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 11

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/6858e2dbf7a5

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8487451 [details] [diff] [review]
Drop the remaining backbone views for Loop (switch to react).

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.