Closed Bug 1220878 Opened 9 years ago Closed 9 years ago

switch from React.addons.classSet to classnames package

Categories

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

defect

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: dmosedale, Assigned: dmosedale)

References

Details

Attachments

(1 file, 3 obsolete files)

In preparation for upgrading to react 0.13.3, we need to replace React.addons.classSet with something else, or else get a bunch of deprecation warnings.  The React folks suggest https://github.com/JedWatson/classnames as a replacement.
This looks pretty good so far, I still need to test it with the dist
configuration.
Updated to work correctly in production builds.
Attachment #8682507 - Flags: review?(edilee)
Attachment #8682256 - Attachment is obsolete: true
Comment on attachment 8682507 [details] [diff] [review]
switch Hello from React.addons.classSet to classNames package

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

::: browser/components/loop/jar.mn
@@ +113,5 @@
>    content/browser/loop/shared/libs/react-0.12.2.js    (content/shared/libs/react-0.12.2-prod.js)
>  #endif
>    content/browser/loop/shared/libs/lodash-3.9.3.js    (content/shared/libs/lodash-3.9.3.js)
>    content/browser/loop/shared/libs/backbone-1.2.1.js  (content/shared/libs/backbone-1.2.1.js)
> +  content/browser/loop/shared/libs/classnames.js      (standalone/node_modules/classnames/index.js)

This isn't going to work for Firefox builds, unless you're going to hook node into the build system...
hah, indeed.  good catch.  I'll just copy it into the libs directory, like we do with react.  new patch forthcoming.
Attachment #8682507 - Flags: review?(edilee)
Rank: 24
Attachment #8682507 - Attachment is obsolete: true
Comment on attachment 8682689 [details] [diff] [review]
switch Hello from React.addons.classSet to classNames package

Asking for license-review from gerv; this is MIT licensed: https://github.com/JedWatson/classnames/blob/master/LICENSE
Attachment #8682689 - Flags: review?(gerv)
Comment on attachment 8682689 [details] [diff] [review]
switch Hello from React.addons.classSet to classNames package

Not sure if it would be worthwhile to just expose classnames as cx globally..
Attachment #8682689 - Flags: review?(edilee) → review+
Comment on attachment 8682689 [details] [diff] [review]
switch Hello from React.addons.classSet to classNames package

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

In reviewing/testing bug 1147167, I noticed that this patch is wrong: The filename is classnames-2.2.0.js, but the {conversation,panel}.html load classnames.js
Attachment #8682689 - Flags: feedback-
Comment on attachment 8682689 [details] [diff] [review]
switch Hello from React.addons.classSet to classNames package

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

No problem - stick it in about:license as usual. <sigh>. We have so many copies of this license, identical apart from the copyright line...

Gerv
Attachment #8682689 - Flags: review?(gerv) → review+
(In reply to Ed Lee :Mardak from comment #7)
> Comment on attachment 8682689 [details] [diff] [review]
> switch Hello from React.addons.classSet to classNames package
> 
> Not sure if it would be worthwhile to just expose classnames as cx globally..

Too terse, I think.  We could rename it, but the existing convention seems good enough to me.
(In reply to Mark Banner (:standard8) from comment #8)
> Comment on attachment 8682689 [details] [diff] [review]
> switch Hello from React.addons.classSet to classNames package
> 
> Review of attachment 8682689 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In reviewing/testing bug 1147167, I noticed that this patch is wrong: The
> filename is classnames-2.2.0.js, but the {conversation,panel}.html load
> classnames.js

Good catch, and the automated testing should have caught it.  Either I forgot to run it, or my tree was in an anomalous state before.  The tests are catching it now, however, and I've updated the code to.
https://hg.mozilla.org/integration/fx-team/rev/df66cac11155fda2f1d336d71dfc529b22636b81
Bug 1220878-switch Hello from React.addons.classSet to classnames package, r=edilee,gerv
Fixed up license.html and hanging file references.
Attachment #8683944 - Flags: review?(gerv)
Attachment #8683944 - Flags: review?(edilee)
Attachment #8682689 - Attachment is obsolete: true
Comment on attachment 8683944 [details] [diff] [review]
switch Hello from React.addons.classSet to classnames package

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

::: toolkit/content/license.html
@@ +2722,5 @@
>      <hr>
>  
> +    <h1><a id="classnames"></a>classnames License</h1>
> +
> +    <p>This license applies to the file <span class="path">

file_s_.

r=gerv.

Gerv
Attachment #8683944 - Flags: review?(gerv) → review+
https://hg.mozilla.org/mozilla-central/rev/df66cac11155
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(In reply to Gervase Markham [:gerv] from comment #14)
> Comment on attachment 8683944 [details] [diff] [review]
> switch Hello from React.addons.classSet to classnames package
> 
> Review of attachment 8683944 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/license.html
> @@ +2722,5 @@
> >      <hr>
> >  
> > +    <h1><a id="classnames"></a>classnames License</h1>
> > +
> > +    <p>This license applies to the file <span class="path">
> 
> file_s_.
> 
> r=gerv.

Whoops; I didn't actually intend to ask for review again.  In fact, it is just one file, and the reason for the * in the name is that the version number is in the number and will change as we upgrade.
Comment on attachment 8683944 [details] [diff] [review]
switch Hello from React.addons.classSet to classnames package

Dropping obsolete review request.
Attachment #8683944 - Flags: review?(edilee)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: