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

Fix false positives for style attributes in no-missing-end-of-source-newline #3485

Merged
merged 1 commit into from Jul 21, 2018

Conversation

3 participants
@gucong3000
Member

gucong3000 commented Jul 20, 2018

Which issue, if any, is this issue related to?

#3410
gucong3000/postcss-html#70

Is there anything in the PR that needs further explanation?

No, it's self explanatory.

@ntwb

ntwb approved these changes Jul 20, 2018

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jul 20, 2018

Interesting.

This makes the rule aware of the context in which it's used in, right? i.e. root.source.inline is a property introduced in postcss-html?

Up until now, I think rules have been unware of their context. It has been the user's responsbility to decide which rules are appropriate to turn on.

A .vue files can contain two types of styles in one file e.g.

<style>
  .component { color: red; }
</style>
<script>
  new Vue({
    el: '#demo',
    render: function (h) {
      return (
        <span style="max-width: 10px">{something_else}</span>
      )
    }
  })
</script>

Wouldn't a user want to turn off this rule for both these instances?

And isn't that better done with an overrides property than this approach?

@gucong3000

This comment has been minimized.

Member

gucong3000 commented Jul 20, 2018

root.source.inline is a property introduced in postcss-html?

Yes. there's something else:

const document.syntax.parse('............')
document.source.lang  // lang of source: html, jsx, markdown, css, sass, scss, less ...
document.nodes[0].source.lang //lang of first style block, only work for html-like, jsx, markdown
@gucong3000

This comment was marked as outdated.

Member

gucong3000 commented Jul 20, 2018

Wouldn't a user want to turn off this rule for both these instances?

We need keep this rule for style tag:

<style>
  .component { color: red; }
  // a newline should be here: ==>>
</style>

styles in jsx in html is not support:

<style>
  .component { color: red; }
</style>
<script>
  new Vue({
    el: '#demo',
    render: function (h) {
      return (
        // I think stylelint not support next line:
        <span style="max-width: 10px">{something_else}</span>
      )
    }
  })
</script>

@gucong3000 gucong3000 force-pushed the inline_style branch 2 times, most recently from 0f038bb to b05dd45 Jul 20, 2018

@gucong3000

This comment has been minimized.

Member

gucong3000 commented Jul 20, 2018

And isn't that better done with an overrides property than this approach?

This is true in other rules. But for this rule, we have a clear disabling condition:

  1. inline styles in HTML
  2. object literal for CSS-in-JS

Users does not need to choose config.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jul 20, 2018

But for this rule, we have a clear disabling condition
Users does not need to choose config.

Ok. I agree it's the better developer experience.

Can you please revert the last commit (adding in JSX)? Then we can merge this.

We should add your last commit in separate PR, like #3405.

@gucong3000 gucong3000 force-pushed the inline_style branch from b05dd45 to bcb4211 Jul 21, 2018

@gucong3000

This comment has been minimized.

Member

gucong3000 commented Jul 21, 2018

Can you please revert the last commit

Done.

@gucong3000 gucong3000 force-pushed the inline_style branch from bcb4211 to 83229d5 Jul 21, 2018

@jeddy3 jeddy3 changed the title from Disable `no-missing-end-of-source-newline` for inline styles to Fix false positives for style attributes in no-missing-end-of-source-newline Jul 21, 2018

@jeddy3

jeddy3 approved these changes Jul 21, 2018

@jeddy3 jeddy3 merged commit 775903a into master Jul 21, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0009%) to 96.009%
Details

@jeddy3 jeddy3 deleted the inline_style branch Jul 21, 2018

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jul 21, 2018

  • Fixed: no-missing-end-of-source-newline false positives for for style attributes (#3485).
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jul 21, 2018

@gucong3000 Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment