Skip to content

Resolved https://github.com/webpack/css-loader/issues/102 #183

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Dec 22, 2015
Merged

Resolved https://github.com/webpack/css-loader/issues/102 #183

merged 5 commits into from
Dec 22, 2015

Conversation

ShyykoSerhiy
Copy link
Contributor

No description provided.

sokra
sokra reviewed Nov 14, 2015
View reviewed changes
lib/loader.js
urlRequest = url;
return "\" + require(" + loaderUtils.stringifyRequest(this, urlRequest) + ") + \"";
}.bind(this));
if(!query.hasOwnProperty('import')) {
Copy link
Member

Choose a reason for hiding this comment

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

Please don't check for exisitance of query properties. Check for the value. query.import !== false. Document it as css-loader?-url&-import.

Elsewise this cases would be broken:

  • css-loader?url
  • css-loader?{url: true}

@sokra
Copy link
Member

sokra commented Nov 14, 2015

Please add test cases that validate the behavior.

@sokra sokra mentioned this pull request Nov 14, 2015
Closed
@ShyykoSerhiy
Copy link
Contributor Author

@sokra I've added some tests and fixed checks

@jasonslyvia
Copy link

Please merge this!

3 similar comments
@xile611
Copy link

xile611 commented Dec 22, 2015

Please merge this!

@arcthur
Copy link

arcthur commented Dec 22, 2015

Please merge this!

@jasonHzq
Copy link

Please merge this!

@shelbeniskb
Copy link

please merge this!

@sokra sokra merged commit c3afc4f into webpack-contrib:master Dec 22, 2015
sokra added a commit that referenced this pull request Dec 22, 2015
@sokra
fixed missing tests and import handling for #183
5651d70
@sokra
Copy link
Member

sokra commented Dec 22, 2015

please merge this!

Instead of spamming please merge this! try to review the PR and comment with broken stuff. I. e. here were tests regarding the @import handling missing. And no tests propably means: behavior is broken.

@vikalpgupta-birdeye
Copy link

@sokra This parameter works for url rewriting inside the stylesheet, but it also does not emit the files, can there be a parameter, in which we can specify such requirement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ShyykoSerhiy @sokra @jasonslyvia @xile611 @arcthur @jasonHzq @shelbeniskb @vikalpgupta-birdeye