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

Fixes #176 by adding css-specific "optimize" option #181

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

daltonjc
Copy link

@daltonjc daltonjc commented Dec 24, 2014

Removed the dependency on requirejs's optimizeCss option and introduced a css-specific option which can disable optimization, defaulting to true. I added this under requirejs's "config" option which allows module-specific options without polluting the "global" config thinking that's the least intrusive option going forward, but happy to change that if desired.

Let me know if anything is amiss.

UPD, by @alundiak: This is try to fix issue #176

Fix guybedford#176

Only perform optimization on the css content if the build's
config.css.optimize option is not set to false.  This is a new config
option, added under require's "config" option which allows specifying
module-specific configuration without polluting the global config
object.
Added usage documentation to the readme for the option to enable/disable
css optimization, for fixing guybedford#176
Minor update to the readme documentation for this new option to read a
bit better
Removing the local test example files I created from the branch since I
stupidly added them to master before creating the PR branch and don't
want them in the PR.
@guybedford
Copy link
Owner

Thanks for this!

I'm not sure about separating the configuration though since the convention is to have separateCSS directly on the base-level configuration.

Perhaps we can use another name that doesn't clash like optimizeRequireCSS?

Changes suggested in pull request to move config.css.optimize to
optimizeRequireCSS since the rest of the require-css config doesn't use
that convention.
@daltonjc
Copy link
Author

Thanks @guybedford. I made the requested change.

@alundiak
Copy link
Collaborator

alundiak commented Feb 2, 2017

@daltonjc if your suggested code changes is ok, and still relevant, please squash commits into one, and update PR. I will review it later and merge/decline. In case of non-activity, PR will be closed, but changes might be incorporated later.

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.

None yet

3 participants