Closed Bug 1065608 Opened 6 years ago Closed 6 years ago

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

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
normal

Tracking

(firefox34 fixed, firefox35 fixed)

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [loop-uplift])

Attachments

(1 file)

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.
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.
Attachment #8487451 - Flags: review?(nperriault)
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/>
Attachment #8487451 - Flags: review?(nperriault) → review+
https://hg.mozilla.org/integration/fx-team/rev/e00245b62a02
Target Milestone: --- → mozilla35
https://hg.mozilla.org/mozilla-central/rev/e00245b62a02
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Does this require any manual testing?
Flags: qe-verify?
Flags: needinfo?(standard8)
Whiteboard: [loop-uplift?]
(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.
Flags: needinfo?(standard8)
Whiteboard: [loop-uplift?] → [loop-uplift]
(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?
Flags: needinfo?(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.
Flags: needinfo?(standard8)
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.
Flags: qe-verify? → qe-verify-
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
Attachment #8487451 - Flags: approval-mozilla-aurora?
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.
Attachment #8487451 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.