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

use node-sass for building sass #2483

Merged
merged 2 commits into from
Sep 19, 2016
Merged

Conversation

koenpunt
Copy link
Collaborator

@koenpunt koenpunt commented Dec 2, 2015

Replaced compass for node-sass and added autoprefixer for automated vendor prefixes.

This removes the ruby dependency, and makes building way faster.

This also resolves the issues people having when including the Chosen source into their rails projects.

Related #2368 #2474

@koenpunt
Copy link
Collaborator Author

@tjschuck @mlettini @pfiller @stof anyone? (I can't tag @harvesthq/developers anymore)

@stof
Copy link
Collaborator

stof commented Dec 16, 2015

this is a great news. I thought about suggesting it the last time I had to build Chosen from source locally

@stof
Copy link
Collaborator

stof commented Dec 16, 2015

👍

@tjschuck
Copy link
Member

How will this fit into the Travis => https://github.com/harvesthq/bower-chosen build system? Seamlessly?

Also, Sass != Compass — were we not using any of the Compass-y bits of Compass?

@koenpunt
Copy link
Collaborator Author

Also, Sass != Compass — were we not using any of the Compass-y bits of Compass?

Just some css mixins which are covered by autoprefixer, and the image-url() helper, which is irrelevant because you can already set the url using a variable, so when you want to use the image-url helper (in whatever framework) you simply include the sass file like this:

$chosen-sprite: image-url('chosen-sprite.png');
$chosen-sprite-retina: image-url('chosen-sprite@2x.png');
@include "chosen";

@koenpunt
Copy link
Collaborator Author

How will this fit into the Travis => https://github.com/harvesthq/bower-chosen build system? Seamlessly?

Yes I think it does, just the ruby dependency is stripped and a few extra node modules are installed through npm.

@koenpunt
Copy link
Collaborator Author

Although I'm not sure if the browser specification 'last 2 versions, IE 8' covers all we do support, so maybe this should be a little more explicit.

@tjschuck
Copy link
Member

For the Ruby dependency => pure Node change, I have pretty much no opinion either way, so cool with me.

@mlettini You have any thoughts on the Compass => pure Sass changes?

@stof
Copy link
Collaborator

stof commented Dec 16, 2015

@koenpunt I think it is OK for the browser specification

@mlettini
Copy link
Contributor

For the Ruby dependency => pure Node change, I have pretty much no opinion either way, so cool with me.

Same boat. Compass was just a tool to allow us to write a property once and not all of it's prefixes for old browser support. Now, autoprefixer is that tool, and that's fine with me. Saves a dependency. I believe we made this change for Harvestapp internally too.

@starzonmyarmz
Copy link
Member

I'm excited for this! 👍

@pfiller
Copy link
Contributor

pfiller commented Mar 24, 2016

This seems great to me. I compared the css before and after and they seem to be effectively the same. I really don't like node-sass's default nested style for output:

whack

My personal vote would be to change the outputStyle setting to 'expanded, but I'd defer to @mlettini or @starzonmyarmz on that.

@koenpunt do you mind merging master in? It's a pretty easy conflict in travis.yml.

@koenpunt
Copy link
Collaborator Author

Should the formatting of the resulting CSS matter? Maybe we should go with convention over configuration here.

I've updated the branch 🚀

@koenpunt
Copy link
Collaborator Author

I'll also squash some all of those commits

@koenpunt
Copy link
Collaborator Author

While at it, we might consider upgrading our node version, 0.10 is a oldie

https://semver.io/node/stable => v5.9.1

@@ -3,7 +3,7 @@ sudo: false
language: node_js

node_js:
- "0.10"
- "5.9.1"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using 5.9 to get the latest patch release automatically

@koenpunt
Copy link
Collaborator Author

@stof good point, updated.

@starzonmyarmz
Copy link
Member

Should the formatting of the resulting CSS matter?

I think we should either go fully compressed or expanded. Personally, I like open source projects to have expanded CSS for educational purposes, and have an optional minified version to go along with it. I think only having the expanded version is fine - there's no reason someone using Chosen can't go minify the file themselves.

@koenpunt
Copy link
Collaborator Author

@starzonmyarmz the minified version is also generated when building, and the resulting file isn't included in the repo anyway.

@pfiller
Copy link
Contributor

pfiller commented Mar 25, 2016

The version we're shipping to bower-chosen is currently not minified and I don't think it should be. I'd prefer we use expanded so we don't make a big diff here:

https://github.com/harvesthq/bower-chosen/blob/master/chosen.css

Plus, I think the nested thing looks worse!

@pfiller pfiller added the target label Jun 3, 2016
Replaced compass for node sass and added autoprefixer for automated vendor prefixes

This removes the ruby dependency

remove compass from package.json

remove gem install from travis config

update contributing guide accordingly
@koenpunt koenpunt merged commit 358ade5 into harvesthq:master Sep 19, 2016
@koenpunt koenpunt deleted the node-sass branch September 20, 2016 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants