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

Add support for CSS and LESS files by default #8 #9

Closed
wants to merge 3 commits into from

Conversation

VovanR
Copy link

@VovanR VovanR commented Jun 17, 2016

No description provided.

@JaKXz
Copy link
Contributor

JaKXz commented Aug 4, 2016

@VovanR awesome contribution 💯

sorry for the delayed response though! Would you mind rebasing your PR? The only conflicts will be Line 65 in index.js and the location of the test files -- there's a helper function now that get's the path from the ./test directory in ./test/helpers/setup.js. Let me know if you have issues, or I can take over this PR when I have a few minutes to spare.

@VovanR VovanR force-pushed the feature/support-css branch from 748de0b to 498dd39 Compare August 4, 2016 08:22
@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage remained the same at 86.486% when pulling 498dd39 on VovanR:feature/support-css into 40be41f on vieron:master.

@VovanR
Copy link
Author

VovanR commented Aug 4, 2016

@JaKXz done.

@JaKXz
Copy link
Contributor

JaKXz commented Aug 4, 2016

@vieron any final thoughts? This should make the 0.3.1 release.

Thanks again @VovanR!

@vieron
Copy link
Contributor

vieron commented Aug 4, 2016

@JaKXz looks good to me! Thanks @VovanR!

@JaKXz I can do both releases (0.3.0 and 0.3.1) in a few hours

@vieron
Copy link
Contributor

vieron commented Aug 4, 2016

Now that I was going to merge, I thought this could be a breaking change for some consumers.

Consumers could use specific syntax in .scss files following rules defined in their .stylelintrc, but not in .css/.less files. I can imagine things like normalize.css or other vendor css libraries breaking build pipelines because of stylelint started to lint .css files.

What do you think @JaKXz @VovanR ?

@JaKXz
Copy link
Contributor

JaKXz commented Aug 4, 2016

Perhaps there should be another test that makes sure those errors aren't thrown if you have mixed style files?

Though I don't think vendor libraries will actually be linted if they're imported or required so I can't think of a real scenario where this would actually be an issue. Please correct me if I'm wrong!

@JaKXz
Copy link
Contributor

JaKXz commented Aug 4, 2016

If necessary, we can make this a breaking change as well, for 0.4.0

@VovanR
Copy link
Author

VovanR commented Aug 4, 2016

I place vendor libs to disable section

/* stylelint-disable */
@import "../bower_components/awesome-bootstrap-checkbox/awesome-bootstrap-checkbox.css";
@import "../bower_components/mdi/css/materialdesignicons.css";
@import "../bower_components/owl.carousel/dist/assets/owl.carousel.css";
@import "../bower_components/owl.carousel/dist/assets/owl.theme.default.css";
@import "../bower_components/fancybox/source/jquery.fancybox.css";
@import "../bower_components/eonasdan-bootstrap-datetimepicker/build/css/bootstrap-datetimepicker.css";
/* stylelint-enable */

I do not think that this is a breaking change

@JaKXz
Copy link
Contributor

JaKXz commented Aug 13, 2016

@VovanR could you update your fixtures to cover the edge cases @vieron mentioned? I have a feeling you're right and it won't be a breaking change, but it would be nice to make sure.

pack(assign({}, baseConfig, config), function (err, stats) {
expect(err).to.not.exist;
expect(stats.compilation.errors.length).to.equal(4);
expect(stats.compilation.warnings.length).to.equal(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the .to.have.length assertion on the errors and warnings arrays, because they provide better messages when failing.

@coveralls
Copy link

coveralls commented Aug 19, 2016

Coverage Status

Coverage remained the same at 87.179% when pulling 9b3d512 on VovanR:feature/support-css into bb02a54 on vieron:master.

@VovanR
Copy link
Author

VovanR commented Aug 19, 2016

@JaKXz done!

@@ -0,0 +1,3 @@
body {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you try using sass specific syntax here?

* Update test with 'block-no-empty' rule
@coveralls
Copy link

coveralls commented Aug 24, 2016

Coverage Status

Coverage remained the same at 87.179% when pulling 969d393 on VovanR:feature/support-css into bb02a54 on vieron:master.

@JaKXz
Copy link
Contributor

JaKXz commented Sep 17, 2017

Closing in favour of #107 - thanks for your work on this.

cc @barraponto

@JaKXz JaKXz closed this Sep 17, 2017
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.

4 participants