Mainly this just involves getting a PR (or patch) for just the panel pieces of the port that are already done, and having that reviewed before the patch grows bigger. :-) We can figure out more reviewing theory tomorrow...

As a very first step, this is the react main library file, with its entry in Loop's jar.mn. I don't know about licensing review/validation, so I'm leaving Dan adding supplementary reviewers appropriately here.

Note: this should only be landed along with the upcoming patch for the Panel.

This is the patch porting Loop's panel views to React, including both the jsx source file and its compiled version as well as updated tests.

Comment on attachment 8450200 [details] [diff] [review] 0001-Bug-1033841-Added-react-0.10.0-to-loop-shared-libs.patch Review of attachment 8450200 [details] [diff] [review]: ----------------------------------------------------------------- r=dmose; conditional on filing a followup MVP-blocker to figure out how we want to handle the "production" vs "dev" builds of react in our tree.

Comment on attachment 8450200 [details] [diff] [review] 0001-Bug-1033841-Added-react-0.10.0-to-loop-shared-libs.patch Review of attachment 8450200 [details] [diff] [review]: ----------------------------------------------------------------- Heads up, this build has a bunch of debug code that gets stripped out in our production build. You may want to conditionally swap in the other build for non-DEBUG builds. I don't know if any of your other libs do the same dev/prod split or if they just minify, but you might want to consider it for those as well.

Comment on attachment 8450284 [details] [diff] [review] 0001-Bug-1033841-Ported-Loop-panel-views-to-React-part-2.patch Review of attachment 8450284 [details] [diff] [review]: ----------------------------------------------------------------- React components look good, hopefully these other comments help! ::: browser/components/loop/content/shared/js/router.js @@ +78,5 @@ > + if (!this._activeView) { > + return; > + } > + if (this._activeView.type === "react") { > + React.unmountComponentAtNode(document.querySelector("#main")); Lots of people forget this, but I'm glad you didn't. React will stay attached as long as that document exists (not as big a problem here, but in long lived single page apps, it results in a unreleased memory). That said, if you're in here from loadReactComponent, you could skip this step of unmounting. If you're rending the same component over the existing one, React can make optimizations to reuse parts (or all) of the existing DOM. If you unmount first, we'll wipe out the DOM then recreate. We'll also wipe out components' state, which might actually be a desired effect. ::: browser/components/loop/content/shared/js/views.js @@ +94,5 @@ > + * @type {Object} > + */ > + var ReactL10nMixin = { > + componentDidMount: function() { > + l10n.translate(this.getDOMNode()); I'm not familiar with how `l10n.translate` works, but based on this call, it's going in and modifying the contents of the DOM node. This can put React into a bad situation and if an update comes along that doesn't result in a mount, but does modify DOM contents, React will blow away the translated content and not re-translate. You could hook into `componentDidUpdate` as well which runs after an update to the subtree, but then you're modifying the DOM directly a lot. The way we've done this at FB and we've seen others do it, is to translate the strings in the render method. eg. render: function() { return <div>{t("hello", CURRENT_LOCALE)}</div> } It looks like you already do that in a few places, so I would strongly encourage always doing that vs DOM mutation approach.

(In reply to Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) from comment #6) > Comment on attachment 8450284 [details] [diff] [review] First, thanks for taking the time to review this code! > That said, if you're in here from loadReactComponent, you could skip this > step of unmounting. We're transiting from Backbone views to React, so we need to use both as a main root component; we unmount a react component before it may be replaced by a Backbone view and the other way around :) > The way we've done this at FB and we've seen others do it, is to translate > the strings in the render method. Updated patch implements just this, thanks for the heads up! Last, I've fixed a broken tests in router_test.js as well.

Comment on attachment 8450284 [details] [diff] [review] 0001-Bug-1033841-Ported-Loop-panel-views-to-React-part-2.patch Review of attachment 8450284 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/content/js/src/panel.jsx @@ +37,5 @@ > + this.setState({doNotDisturb: navigator.mozLoop.doNotDisturb}); > + }, > + > + render: function() { > + var status = this.state.doNotDisturb ? 'Unavailable' : 'Available'; We need to get this localized before landing. Also, please remove the old l10n string. @@ +138,5 @@ > + this.props.notifier.errorL10n("unable_retrieve_url"); > + return; > + } > + > + this.props.notifier.clear(); If the notifier can display more than 1 notification at a time, this presumably needs to happen before the if statement. @@ +141,5 @@ > + > + this.props.notifier.clear(); > + > + this.setState({ > + pending: false, pending also needs to be set to false in the error case; and it'd be good to have a test for this too. @@ +165,5 @@ > + className={cx({'pending': this.state.pending})} > + data-l10n-id="caller" onChange={this.handleTextChange} /> > + > + <button type="submit" className="get-url btn btn-success" > + disabled={cx({"disabled": this.state.disabled})} This might be worth a comment on why using cx here makes sense, since this is not a class. Standard8 and I puzzled it out after a bit, but it's not entirely obvious. @@ +180,5 @@ > + var PanelView = React.createClass({ > + mixins: [sharedViews.ReactL10nMixin], > + > + componentWillMount: function() { > + this.notifier = this.props.notifier; Presumably there's no need for the this.notifier shorthand, since it's only used once. ::: browser/components/loop/content/shared/js/router.js @@ +78,5 @@ > + if (!this._activeView) { > + return; > + } > + if (this._activeView.type === "react") { > + React.unmountComponentAtNode(document.querySelector("#main")); Right now we can't make the assumption that we'll be in here from loadReactComponent, as we still have some backbone views. However, soon we will be able to. That said, I'm not sure we ever load the same component on top of itself. But this semantic thing is super good to know, thanks Paul. I'd suggest we file a spin-off bug to consider removing amount again later.

(In reply to Dan Mosedale (:dmose - needinfo? me for responses) from comment #8) > We need to get this localized before landing. Also, please remove the old > l10n string. Ah, sure, and I've seen Darrin commenting elsewhere that the proper wording was "Do Not Disturb" as per the mockups and as it was previously implemented. Will fix. > @@ +138,5 @@ > > + this.props.notifier.errorL10n("unable_retrieve_url"); > > + return; > > + } > > + > > + this.props.notifier.clear(); > > If the notifier can display more than 1 notification at a time, this > presumably needs to happen before the if statement. Nope because of the early return in that if statement. > > + this.setState({ > > + pending: false, > > pending also needs to be set to false in the error case; and it'd be good to > have a test for this too. That's true, will fix. > > + <button type="submit" className="get-url btn btn-success" > > + disabled={cx({"disabled": this.state.disabled})} > > This might be worth a comment on why using cx here makes sense, since this > is not a class. Standard8 and I puzzled it out after a bit, but it's not > entirely obvious. Well the classSet function is probably poorly named as it's much more like a conditional string generator, but naming things is hard. Will add a comment. > > + componentWillMount: function() { > > + this.notifier = this.props.notifier; > > Presumably there's no need for the this.notifier shorthand, since it's only > used once. Will fix.

(In reply to Nicolas Perriault (:NiKo`) from comment #9) > > > + <button type="submit" className="get-url btn btn-success" > > > + disabled={cx({"disabled": this.state.disabled})} > > > > This might be worth a comment on why using cx here makes sense, since this > > is not a class. Standard8 and I puzzled it out after a bit, but it's not > > entirely obvious. > > Well the classSet function is probably poorly named as it's much more like a > conditional string generator, but naming things is hard. Will add a comment. You can actually just pass a boolean and React will output the right thing (in this case the HTML will look like "<button ... disabled ...>" since disabled is a valueless presence-has-meaning attribute). React will actually treat your string as a bool and look for truthiness, "disabled" == true, "" == false.

Comment on attachment 8450588 [details] [diff] [review] 0001-Bug-1033841-Ported-Loop-panel-views-to-React-part2-rev2.patch Review of attachment 8450588 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/content/shared/css/panel.css @@ +50,5 @@ > } > > +p.dnd { > + margin: 0 10px 10px 10px; > + padding-bottom: 10px; I'm curious why it's necessary to use two rules here rather than just "margin: 10px;". ::: browser/components/loop/test/desktop-local/panel_test.js @@ +168,3 @@ > }); > > + describe("#handleCheckboxChange", function() { It seems like the describe block really wants to talk about the "checkbox change event" rather than the individual router that happens to handle it. @@ +191,3 @@ > beforeEach(function() { > + callUrlData = { > + call_url: "http://call.me/", It's generally considered good practice to use guaranteed-unresolvable URLs as test throw-aways in case there's a bug. Eg "http://call.invalid/". @@ +207,3 @@ > }); > > + describe("#handleFormSubmit", function() { Again, I think we want the describe blocks to talk about units of interface behavior rather than units of implementation code.

> > @@ +138,5 @@ > > > + this.props.notifier.errorL10n("unable_retrieve_url"); > > > + return; > > > + } > > > + > > > + this.props.notifier.clear(); > > > > If the notifier can display more than 1 notification at a time, this > > presumably needs to happen before the if statement. > > Nope because of the early return in that if statement. Isn't the early return exactly the reason it's needed? I.e. couldn't the user end up with both an old notification and an "unable_retrieve_url" notification?

In general, this looks great. I'd like Standard8 to look over changes you make between now and landing and have him put the final r+ on it. W00t!

Patch updated, comments below. (In reply to Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) from comment #10) > You can actually just pass a boolean and React will output the right thing > (in this case the HTML will look like "<button ... disabled ...>" Nice, thank you for the hint! (In reply to Dan Mosedale (:dmose - needinfo? me for responses) from comment #11) > > + margin: 0 10px 10px 10px; > > + padding-bottom: 10px; > > I'm curious why it's necessary to use two rules here rather than just > "margin: 10px;". Because for some reason, panel won't increase its height when using a bottom margin, while it works using a padding. Dunno if it's a bug or not. Added a comment about that. (In reply to Dan Mosedale (:dmose - needinfo? me for responses) from comment #12) > Isn't the early return exactly the reason it's needed? I.e. couldn't the > user end up with both an old notification and an "unable_retrieve_url" > notification? Oh now I see what you mean. Fixed.

Comment on attachment 8450773 [details] [diff] [review] 0001-Bug-1033841-Ported-Loop-panel-views-to-React-part2-rev3.patch So rebasing the patch on top of latest master it conflicts hard… Let me cancel the review for now and port the last introduced bits to React (namely ToSView).

Patch rebased on top of latest master. I had to fix a few glitches as well, as discussed on IRC with Mark.

Comment on attachment 8450960 [details] [diff] [review] 0001-Bug-1033841-Ported-Loop-panel-views-to-React-part2-rev4.patch Review of attachment 8450960 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/content/js/src/panel.jsx @@ +35,5 @@ > + this.setState({doNotDisturb: navigator.mozLoop.doNotDisturb}); > + }, > + > + render: function() { > + var status = this.state.doNotDisturb ? 'Unavailable' : 'Available'; The L10n here still needs fixing. ::: browser/components/loop/content/panel.html @@ +8,4 @@ > <title>Loop Panel</title> > <link rel="stylesheet" type="text/css" href="loop/shared/css/common.css"> > <link rel="stylesheet" type="text/css" href="loop/shared/css/panel.css"> > + <script type="text/javascript" src="loop/shared/libs/react-0.10.0.js"></script> Any reason why this isn't loaded later alongside the rest of the scripts? ::: browser/components/loop/test/desktop-local/index.html @@ +7,4 @@ > <meta charset="utf-8"> > <title>Loop desktop-local mocha tests</title> > <link rel="stylesheet" media="all" href="../shared/vendor/mocha-1.17.1.css"> > + <script type="text/javascript" src="../../content/shared/libs/react-0.10.0.js"></script> Ditto here wrt where to load.

Comment on attachment 8450960 [details] [diff] [review] 0001-Bug-1033841-Ported-Loop-panel-views-to-React-part2-rev4.patch Review of attachment 8450960 [details] [diff] [review]: ----------------------------------------------------------------- No further comments other than the previous.

(In reply to Mark Banner (:standard8) from comment #17) > The L10n here still needs fixing. Fixed. > Any reason why this isn't loaded later alongside the rest of the scripts? > Ditto here wrt where to load. Fixed.

Comment on attachment 8451035 [details] [diff] [review] 0001-Bug-1033841-Ported-Loop-panel-views-to-React-part2-rev5.patch Review of attachment 8451035 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, r=Standard8

https://hg.mozilla.org/integration/mozilla-inbound/rev/56ba52f28300 https://hg.mozilla.org/integration/mozilla-inbound/rev/f6016481c7f2

Backed out for xpcshell test failures on various platforms, see https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=94b064ee68b2 , for example https://tbpl.mozilla.org/php/getParsedLog.php?id=43154313&tree=Mozilla-Inbound . https://hg.mozilla.org/integration/mozilla-inbound/rev/663639207c42

Relanded with a minor tweak to the xpcshell-test to fix the bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/8611e3389ae4 https://hg.mozilla.org/integration/mozilla-inbound/rev/e82fcb8d8ac6

https://hg.mozilla.org/mozilla-central/rev/8611e3389ae4 https://hg.mozilla.org/mozilla-central/rev/e82fcb8d8ac6

I'm wondering if adding target="_blank" only to 1/2 of the links is wanted. Also, I'd really wish we had less HTML in those strings and use Javascript to manage replacements.

(In reply to Francesco Lodolo [:flod] from comment #25) > I'm wondering if adding target="_blank" only to 1/2 of the links is wanted. > > Also, I'd really wish we had less HTML in those strings and use Javascript > to manage replacements. I've moved these to bug 1034841 which needs to fix the first item you reported anyway.

Untracking for QA. This will in effect be tested via smokestesting. Please needinfo me to request specific testing.