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

minify css via with clean-css or csso #688

Merged
merged 3 commits into from
May 12, 2014

Conversation

luto
Copy link
Contributor

@luto luto commented May 11, 2014

Rework of #484 to statisfy the change-requests.

@luto
Copy link
Contributor Author

luto commented May 11, 2014

  • Just sayin': We should call the option just minifyCSS. cleanCSSConfig is too non-descriptive.
  • Yes i like this, I don't like hard-coding the config property as cleanCSSconfig.
  • i don't really like tying out css minifier to a specific tool.

@stefanpenner
Copy link
Contributor

can you ad a changelog entry?

@luto
Copy link
Contributor Author

luto commented May 11, 2014

Done.

PR for gh-pages is in the works.

@MajorBreakfast
Copy link
Contributor

  • It should be enabled by default for production builds
  • minifyCSS: true should enable it, I think there is nothing wrong with making it configurable by passing in an options object but minifyCSS: true is I think the main use case because it's straightforward

Thx for working on this feature!

@luto
Copy link
Contributor Author

luto commented May 11, 2014

It is enabled for production by default.. I guess you want to be able to disable it via minifyCSS: false? This key is currently being used to pass options to the minifer. So I propose to change it to:

{ "minifyCSS": {
  "enabled": true,
  "options": { }
} }

Would that be okay with you?

@MajorBreakfast
Copy link
Contributor

Ah yes now I understand. We're only talkin' production. Yes, then minifyCSS: false would be great. There is always somebody who wants to opt out of this kind of feature.

@luto
Copy link
Contributor Author

luto commented May 11, 2014

Done. I have also clarifed that this is for production only, over in the documentation-PR: #689

@MajorBreakfast
Copy link
Contributor

For what is the enabled key in between? Makes it more complicated to use.

@luto
Copy link
Contributor Author

luto commented May 11, 2014

Take a look at my comment from half an hour ago.. It is needed because the options for the minifer have to go somewhere.

@MajorBreakfast
Copy link
Contributor

Can't we simply skip the minification step if it's false? The minifier is then a nop anyway.

Sry for being so pesky. :) Ease of use you know...

@luto
Copy link
Contributor Author

luto commented May 11, 2014

This way you can easily disable the minifer without losing your config. The options object can be left out so the line becomes minifyCSS: { disable: false }, if you never ever want to minify your css. This is short enough, imho.

@luto
Copy link
Contributor Author

luto commented May 12, 2014

@stefanpenner anything else besides the changelog?

@stefanpenner
Copy link
Contributor

@luto nope: (it does need a rebase though, as it no longer merges cleanly)

@luto
Copy link
Contributor Author

luto commented May 12, 2014

done

stefanpenner added a commit that referenced this pull request May 12, 2014
minify css via with clean-css or csso
@stefanpenner stefanpenner merged commit 4e91234 into ember-cli:master May 12, 2014
@luto luto deleted the css-minify-2 branch May 12, 2014 17:31
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.

3 participants