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)
Hello (Loop)
Client
Tracking
(firefox45 fixed)
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: dmosedale, Assigned: dmosedale)
References
Details
Attachments
(1 file, 3 obsolete files)
50.11 KB,
patch
|
gerv
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
This looks pretty good so far, I still need to test it with the dist configuration.
Assignee | ||
Comment 2•9 years ago
|
||
Updated to work correctly in production builds.
Attachment #8682507 -
Flags: review?(edilee)
Assignee | ||
Updated•9 years ago
|
Attachment #8682256 -
Attachment is obsolete: true
Comment 3•9 years ago
|
||
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•9 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•9 years ago
|
Attachment #8682507 -
Flags: review?(edilee)
Assignee | ||
Updated•9 years ago
|
Rank: 24
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8682689 -
Flags: review?(edilee)
Assignee | ||
Updated•9 years ago
|
Attachment #8682507 -
Attachment is obsolete: true
Assignee | ||
Comment 6•9 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•9 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 8•9 years ago
|
||
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 9•9 years ago
|
||
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+
Assignee | ||
Comment 10•9 years ago
|
||
(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.
Assignee | ||
Comment 11•9 years ago
|
||
(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.
Assignee | ||
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/df66cac11155fda2f1d336d71dfc529b22636b81 Bug 1220878-switch Hello from React.addons.classSet to classnames package, r=edilee,gerv
Assignee | ||
Comment 13•9 years ago
|
||
Fixed up license.html and hanging file references.
Attachment #8683944 -
Flags: review?(gerv)
Attachment #8683944 -
Flags: review?(edilee)
Assignee | ||
Updated•9 years ago
|
Attachment #8682689 -
Attachment is obsolete: true
Comment 14•9 years ago
|
||
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•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/df66cac11155
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Assignee | ||
Comment 16•9 years ago
|
||
(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 17•9 years ago
|
||
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.
Description
•