switch from React.addons.classSet to classnames package

RESOLVED FIXED in Firefox 45

Status

P2
normal
Rank:
24
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dmose, Assigned: dmose)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
(Assignee)

Comment 1

3 years ago
Created attachment 8682256 [details] [diff] [review]
switch Hello from React.addons.classSet to classNames package

This looks pretty good so far, I still need to test it with the dist
configuration.
(Assignee)

Comment 2

3 years ago
Created attachment 8682507 [details] [diff] [review]
switch Hello from React.addons.classSet to classNames package

Updated to work correctly in production builds.
Attachment #8682507 - Flags: review?(edilee)
(Assignee)

Updated

3 years ago
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...
(Assignee)

Comment 4

3 years ago
hah, indeed.  good catch.  I'll just copy it into the libs directory, like we do with react.  new patch forthcoming.
(Assignee)

Updated

3 years ago
Attachment #8682507 - Flags: review?(edilee)
(Assignee)

Updated

3 years ago
Rank: 24
(Assignee)

Comment 5

3 years ago
Created attachment 8682689 [details] [diff] [review]
switch Hello from React.addons.classSet to classNames package
Attachment #8682689 - Flags: review?(edilee)
(Assignee)

Updated

3 years ago
Attachment #8682507 - Attachment is obsolete: true
(Assignee)

Comment 6

3 years ago
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 7

3 years ago
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
Created attachment 8683944 [details] [diff] [review]
switch Hello from React.addons.classSet to classnames package

Fixed up license.html and hanging file references.
Attachment #8683944 - Flags: review?(gerv)
Attachment #8683944 - Flags: review?(edilee)
(Assignee)

Updated

3 years ago
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+

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/df66cac11155
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
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.