Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

[Feature] Added support for linting style tags within html files #142

Merged

Conversation

blake-newman
Copy link
Contributor

@blake-newman blake-newman commented Apr 5, 2016

  • Added grammar styles for embedded css
  • Added option to enable html linting

Fixes #8.

@@ -35,7 +35,7 @@
"atom-package-deps": "^4.0.1",
"cosmiconfig": "^1.1.0",
"deep-assign": "^2.0.0",
"stylelint": "5.2.1",
"stylelint": "NEEDS UPDATING WHEN AVAILABLE",
Copy link
Member

Choose a reason for hiding this comment

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

Don't put this in, we'll simply not merge this until stylelint has been updated.

@Arcanemagus
Copy link
Member

Can you add a spec testing that this is working?

@Arcanemagus
Copy link
Member

Thanks for getting this started btw!

@gaurav21r
Copy link

@Arcanemagus Now that @blake-newman has successfully merged his code into stylelint/stylelint#1019 Can we go ahead with this?

For projects that use Polymer / WebComponents. Scoping <style> within the HTML is sometimes the only way to go! And stylelint linting only .css files touches across only a small percentage of styles in our project!

@Arcanemagus
Copy link
Member

@gaurav21r This still needs a spec, and a released version of stylelint incorporating it before this can be merged 😉.

@blake-newman
Copy link
Contributor Author

@Arcanemagus @gaurav21r Will be updating this in the coming few days.

@Arcanemagus
Copy link
Member

@blake-newman I'd like to get this released soon, we're you planning on addressing the comments above and updating it for how the final implementation ended up? I can just pull your commit into a new PR and update it there if you don't have time right now 😉.

@blake-newman blake-newman force-pushed the feature/html-style-linting branch from e22107c to e30397f Compare May 8, 2016 14:49
@blake-newman blake-newman changed the title [Feature] [WIP] Added support for linting style tags within html files [Feature] Added support for linting style tags within html files May 8, 2016
@blake-newman
Copy link
Contributor Author

@Arcanemagus All is done :)

"stylelint-config-standard": "^7.0.0"
"deep-assign": "^2.0.0",
"stylelint": "^6.3.2",
"stylelint-config-standard": "^4.0.1"
Copy link
Member

Choose a reason for hiding this comment

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

It seems that master branch version is correct.

@blake-newman blake-newman force-pushed the feature/html-style-linting branch from e30397f to 10bf057 Compare May 8, 2016 18:58
"lazy-req": "^1.1.0",
"stylelint": "6.3.2",
"stylelint-config-standard": "^7.0.0"
"deep-assign": "^2.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

It looks like your package.json changes are simply a reversion to a much older version... deep-assign was moved away from since it has many bugs.

@blake-newman
Copy link
Contributor Author

@Arcanemagus looks as if when i rebased, i incorrectly resolved conflicts. All is resolved now.

@Arcanemagus
Copy link
Member

Okay, looking over the code now 😛.

@@ -24,6 +27,7 @@ describe('The stylelint provider for Linter', () => {
atom.workspace.destroyActivePaneItem();
atom.config.set('linter-stylelint.useStandard', true);
atom.config.set('linter-stylelint.disableWhenNoConfig', false);
atom.config.set('linter-stylelint.enableHtmlLinting', false);
Copy link
Member

Choose a reason for hiding this comment

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

The default value is false, no need to explicitly set it to false here.

Copy link
Member

Choose a reason for hiding this comment

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

I'm still seeing this in the diff?

@Arcanemagus
Copy link
Member

LGTM after those minor nitpicks are addressed. The only thing I can think of that this isn't testing is whether SCSS or Less are linted properly in extracted HTML, but I'm perfectly fine with leaving that testing to stylelint.

- Added component option resolving (async to sync conversion)
- Added grammar styles for embedded css
- Added option to enable html linting
@blake-newman
Copy link
Contributor Author

@Arcanemagus I tested placing atom.config.set('linter-stylelint.enableHtmlLinting', true); once in describe, this took no effect. Removed from beforeEach.

If setting extractStyleTagsFromHtml and anything is passed to stylelint then everything is working, i believe the coverage should suffice.

@Arcanemagus
Copy link
Member

Commented again on those two parts

@Arcanemagus
Copy link
Member

Hmmm, I don't think this actually addresses one of my original comments: If HTML linting is enabled, and then disabled, does this still trigger on embedded CSS?

},
enableHtmlLinting: {
title: 'Enable linting of styles within html',
description: 'Turn on linting of your styles tages within html files.',
Copy link
Member

Choose a reason for hiding this comment

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

"style tags"

@Arcanemagus
Copy link
Member

provideLinter is only called once: When the service is registered with linter. The grammar scopes need to be updated in a manner similar to how linter-eslint does it.

@Arcanemagus
Copy link
Member

Merging this and fixing separately as I would like to get this released.

Thanks for putting this together @blake-newman!

@Arcanemagus Arcanemagus merged commit 6570914 into AtomLinter:master May 8, 2016
@Arcanemagus
Copy link
Member

Fixes are up in #194.

@gaurav21r
Copy link

Thanks a TON @Arcanemagus @blake-newman 👍 👍 👍 👍 💯 / 💯

@blake-newman blake-newman deleted the feature/html-style-linting branch May 16, 2016 08:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants