Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch CSS to using postcss, and implement a dark theme. #2973

Merged
merged 24 commits into from Jan 18, 2017

Conversation

@ara4n
Copy link
Member

ara4n commented Jan 17, 2017

I know this doesn't merge yet, but wanted to get the PR started for initial review.
The rationale for switching to postcss is to get imports, variable expansion & autoprefixing. Originally this was SASS, but SASS doesn't give you autoprefixing and is a bit less flexible than postcss.
Themes are done using ye olde rel="alternate stylesheet".

ara4n added 20 commits Dec 28, 2016
SVGs are all broken, and some of the more exotic colours haven't been updated. There's been no attempt to use SASS to remove duplication from the CSS yet.
no attempt to switch it at runtime yet.
@ara4n ara4n changed the title Switch CSS to using the postcss, and implement a dark theme. Switch CSS to using postcss, and implement a dark theme. Jan 17, 2017
@@ -112,8 +113,17 @@
"minimist": "^1.2.0",
"mkdirp": "^0.5.1",
"mocha": "^2.4.5",
"node-sass": "^4.1.1",

This comment has been minimized.

Copy link
@richvdh

richvdh Jan 17, 2017

Member

if we're not using SASS, let's not drag it in.

We can lose catw too I think.

This comment has been minimized.

Copy link
@ara4n

ara4n Jan 18, 2017

Author Member

ooops, thanks.

@@ -15,13 +15,8 @@ limitations under the License.
*/

.mx_FilePanel {
-webkit-box-ordinal-group: 2;

This comment has been minimized.

Copy link
@richvdh

richvdh Jan 17, 2017

Member

it'd probably be good to keep refactoring the CSS separate from cleaning it up as much as possible, if nothing else to give an easier way to point blame if it starts doing something weird.

This comment has been minimized.

Copy link
@richvdh

richvdh Jan 17, 2017

Member

oh, I might have misunderstood what's going on here - these are now automatically added by postcss?

I still wonder if we can have separate PRs: (1) start using postcss; (2) make use of it. But I can see it may not be worth it.

This comment has been minimized.

Copy link
@ara4n

ara4n Jan 18, 2017

Author Member

Ideally yes, the postcss transition would be separate PR to the factoring out colours, and stripping out vendor prefixes, and theming. Unfortunately they all grew organically at the same time and I'm not convinced it's worth the faff to untangle them now. Future changes should be more reviewable.


echo "// autogenerated by rethemendex.sh" > _components.scss

for i in `find . -iname _\*.scss | fgrep -v _components.scss`;

This comment has been minimized.

Copy link
@richvdh

richvdh Jan 17, 2017

Member

need more sort, please

This comment has been minimized.

Copy link
@ara4n

ara4n Jan 18, 2017

Author Member

done

@@ -30,7 +30,6 @@ require('babel-polyfill');

// CSS requires: just putting them here for now as CSS is going to be

This comment has been minimized.

Copy link
@richvdh

richvdh Jan 17, 2017

Member

could do with a better explanation of what's going on here.

This comment has been minimized.

Copy link
@ara4n

ara4n Jan 18, 2017

Author Member

i have absolutely no idea what's going on there; it predates me.

This comment has been minimized.

Copy link
@richvdh

richvdh Jan 18, 2017

Member

Not sure that's an excuse for making it worse: for a start I think it is no longer the case that CSS is going to be refactored soon, since I think this is the refactoring alluded to.

Perhaps @dbkr can clarify, but I think you could getting rid of this comment and replace it with:

"Require common CSS here; this will make webpack process it into bundle.css. Our own CSS (which is themed) is imported via separate webpack entry points in webpack.config.js"

This comment has been minimized.

Copy link
@ara4n

ara4n Jan 18, 2017

Author Member

happy to change the comment, but would defn prefer to understand what @dbkr had in mind in the first place

This comment has been minimized.

Copy link
@dbkr

dbkr Jan 18, 2017

Member

This was pulling in all the CSS: components.css is the catted-together bundle of all the components' CSS and the others were various other ones we used. Requiring them here pulled them in to webpack's build process which pulled them through the extract text plugin, eventually getting the end CSS bundle out the other end.

The comment was me putting the CSS requires in the vector index file rather than working out if there was a better place to put them, as at the time there was a refactor on the horizon of how vector's CSS worked (although that never came to pass).

This comment has been minimized.

Copy link
@dbkr

dbkr Jan 18, 2017

Member

TL;DR yes, I agree with @richvdh 's new comment

Copy link
Member

richvdh left a comment

generally looks pretty sane to me. A few minor nits.

@richvdh richvdh assigned ara4n and unassigned richvdh Jan 17, 2017
@ara4n ara4n assigned richvdh and unassigned ara4n Jan 18, 2017
@ara4n

This comment has been minimized.

Copy link
Member Author

ara4n commented Jan 18, 2017

@richvdh ptal

if (file.match(/^theme-(?!light\.)/)) {
var match = file.match(/^bundles\/.*?\/theme-((?!light\.).*)\.css$/);
if (match) {
var title = match[1].charAt(0).toUpperCase() + match[1].slice(1);

This comment has been minimized.

Copy link
@richvdh

richvdh Jan 18, 2017

Member

Not convinced that this is anything to do with my webpack changes, but I won't split hairs.

Copy link
Member

richvdh left a comment

LGTM apart from complaining about comments

@richvdh richvdh assigned ara4n and unassigned richvdh Jan 18, 2017
@ara4n ara4n merged commit c0e5a1b into develop Jan 18, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.