Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(349)

Issue 29710555: Issue 6426 - Start using CSS modularization tool in adblockplusui (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 11 months ago by a.giammarchi
Modified:
1 year, 11 months ago
Visibility:
Public.

Description

Issue 6426 - Start using CSS modularization tool in adblockplusui

Patch Set 1 #

Total comments: 23
Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -6 lines) Patch
M .hgignore View 1 chunk +2 lines, -1 line 1 comment Download
M css/desktop-options.css View 2 chunks +1 line, -2 lines 0 comments Download
M js/desktop-options.js View 1 chunk +3 lines, -1 line 7 comments Download
M package.json View 1 chunk +3 lines, -2 lines 15 comments Download

Messages

Total messages: 15
saroyanm
https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED ...
1 year, 11 months ago (2018-02-27 12:06:02 UTC) #1
a.giammarchi
https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED ...
1 year, 11 months ago (2018-02-27 12:31:05 UTC) #2
saroyanm
https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED ...
1 year, 11 months ago (2018-02-27 12:32:58 UTC) #3
a.giammarchi
https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED ...
1 year, 11 months ago (2018-02-27 12:55:19 UTC) #4
a.giammarchi
https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED ...
1 year, 11 months ago (2018-02-27 13:20:59 UTC) #5
a.giammarchi
https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED ...
1 year, 11 months ago (2018-02-27 13:25:55 UTC) #6
Thomas Greiner
https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js#newcode23 js/desktop-options.js:23: require("../css/desktop-options.css"); We already declare it in desktop-options.html so why ...
1 year, 11 months ago (2018-02-27 13:59:21 UTC) #7
saroyanm
https://codereview.adblockplus.org/29710555/diff/29710556/package.json File package.json (right): https://codereview.adblockplus.org/29710555/diff/29710556/package.json#newcode11 package.json:11: "bundle:desktop-options": "eslint ./js/**/*.js && echo '/* eslint-disable */// BUNDLED ...
1 year, 11 months ago (2018-02-27 14:15:19 UTC) #8
a.giammarchi
hopefully answered all questions, glad it works for both now. https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js#newcode23 ...
1 year, 11 months ago (2018-02-27 14:19:36 UTC) #9
Thomas Greiner
https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js#newcode23 js/desktop-options.js:23: require("../css/desktop-options.css"); The problem I see is that this approach ...
1 year, 11 months ago (2018-02-27 19:41:54 UTC) #10
a.giammarchi
https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js#newcode23 js/desktop-options.js:23: require("../css/desktop-options.css"); On 2018/02/27 19:41:54, Thomas Greiner wrote: > The ...
1 year, 11 months ago (2018-02-28 08:10:19 UTC) #11
Thomas Greiner
https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js#newcode23 js/desktop-options.js:23: require("../css/desktop-options.css"); On 2018/02/28 08:10:18, a.giammarchi wrote: > 1 and ...
1 year, 11 months ago (2018-02-28 12:23:14 UTC) #12
a.giammarchi
https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js File js/desktop-options.js (right): https://codereview.adblockplus.org/29710555/diff/29710556/js/desktop-options.js#newcode23 js/desktop-options.js:23: require("../css/desktop-options.css"); On 2018/02/28 12:23:14, Thomas Greiner wrote: > On ...
1 year, 11 months ago (2018-02-28 13:01:04 UTC) #13
Thomas Greiner
I think the crux of this review is that it's not only introducing CSS modules/bundles ...
1 year, 11 months ago (2018-03-07 13:10:22 UTC) #14
a.giammarchi
1 year, 11 months ago (2018-03-07 13:20:31 UTC) #15
On 2018/03/07 13:10:22, Thomas Greiner wrote:
> I think the crux of this review is that ...

is that I've should've put the follow up review but I didn't.
https://codereview.adblockplus.org/29712640/

This can be closed if we like/agree on the other one, which is simpler and keep
things where these belongs.

Apologies for the misunderstanding.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5