Skip to content

Fixes issue #163 #164

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

Closed
wants to merge 1 commit into from
Closed

Fixes issue #163 #164

wants to merge 1 commit into from

Conversation

leog
Copy link

@leog leog commented Aug 25, 2014

Using same check to see if the current load should be import or link in order to use toUrl API or not respectively. Extracted adding suffix ".css" as common denominator.

Issue #163

@alundiak
Copy link
Collaborator

alundiak commented Feb 2, 2017

@leog please squash your commits into one, and update PR, so that we have clear git history in future.
In case of non- activity, I will incorporate suggested changes, and will close this PR.

@leog
Copy link
Author

leog commented Feb 2, 2017

done @alundiak, hope it's what you expected, otherwise let me know

bower.json Outdated
@@ -1,6 +1,6 @@
{
"name": "require-css",
"version": "0.1.4",
"version": "0.1.8",
Copy link
Collaborator

@alundiak alundiak Feb 3, 2017

Choose a reason for hiding this comment

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

@leog Why do u update to 0.1.8, when latest master does have this already? Looks like u didn't pull latest master or rebased. Have u? The same with package.json. Ideally, if u pull/rebase this file should NOT be in changed list. Please check.

css-builder.js Outdated
global._requirejsCssData.usedBy.css = true;
}

if (config.buildCSS != false) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why this codebase was NOT checked by jshint/eslint/jsbeautifier before, but could u please change to !== usage?

Note:

jshint css-builder.js latest from master:

css-builder.js: line 22, col 16, 'csso' used out of scope.
css-builder.js: line 46, col 11, 'file' is already defined.
css-builder.js: line 144, col 40, Use '===' to compare with '0'.
css-builder.js: line 152, col 4, Missing semicolon.
css-builder.js: line 158, col 4, Missing semicolon.
css-builder.js: line 173, col 8, Missing semicolon.
css-builder.js: line 178, col 25, Use '!==' to compare with 'false'.
css-builder.js: line 195, col 4, Missing semicolon.
css-builder.js: line 220, col 30, Use '!==' to compare with 'false'.
css-builder.js: line 220, col 64, Use '!==' to compare with 'true'.
css-builder.js: line 223, col 23, Use '===' to compare with ''.
css-builder.js: line 227, col 11, Misleading line break before '+'; readers may interpret this as an expression boundary.
css-builder.js: line 234, col 4, Missing semicolon.

css-builder.js Outdated
if (config.writeCSSModule && style) {
if (writeCSSForLayer) {
writeCSSForLayer = false;
write(writeCSSDefinition);
Copy link
Collaborator

Choose a reason for hiding this comment

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

line 182 looks not very good aligned.

css-builder.js Outdated
}
else {
cssModule = 'define(function(){})';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

line 188 looks not very good aligned.

@leog
Copy link
Author

leog commented Feb 3, 2017

Sorry @alundiak, now it looks better. Please review.

@alundiak
Copy link
Collaborator

alundiak commented Feb 18, 2017

This looks related to #209 issue.
And this PR is very similar as #198

@leog leog closed this Jun 19, 2024
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.

2 participants