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

⭐️New: Add vue/html-content-newline #445

Closed
wants to merge 7 commits into from

Conversation

ota-meshi
Copy link
Member

This PR adds vue/html-content-newline rule.
This implements rule proposed in #415

@michalsnik michalsnik changed the title [New] Add vue/html-content-newline ⭐️New: Add vue/html-content-newline Apr 1, 2018
@michalsnik
Copy link
Member

michalsnik commented Jul 30, 2018

We've talked with @chrisvfritz and this rule caused a really vivid conversation. In result we came up with an updated idea. What if we name it html-tag-attrs-content-new-line, and force putting content on new line whenever the HTML tag has any attribute, so that we don't mix the content and attributes on the same line? The rule would be very simple to use, it would either be ON or OFF.

Incorrect code:

<label class="label">Lorem ipsum</label>

<div
  class="panel"
>content</div>

Correct code:

<label>Lorem ipsum</label>

<div class="panel">
  content
</div>

What do you think about it guys @ota-meshi @alendorff?

@maksnester
Copy link

we name it html-tag-attrs-content-new-line, and force putting content on new line whenever the HTML tag has any attribute

I think naming is minor thing, because rules are typically googled by use-cases or read directly at the documentation... But about behavior, I think it shouldn't conflict with the rule max-attributes-per-line.

For example I currently have such config:

    "vue/max-attributes-per-line": [
      2,
      {
        "singleline": 2,
        "multiline": {
          "max": 1,
          "allowFirstLine": true
        }
      }
    ]

And this is valid examples:

<div arg1 arg2>Valid</div>

<div arg1
     arg2
     arg3
>
  Valid too
</div>

So if I'll apply this new rule I would still expect the same behavior. But the rule should help to fight with different code which I've mentioned at the issue.
So I would expect the following code as a valid output:

<div arg1>Valid</div>

<div arg1 arg2>Valid</div>

<div arg1>
  Valid
</div>

<div arg1 arg2>
  Valid
</div>

<div arg1 
     arg2
>
  Valid
</div>

@chrisvfritz
Copy link
Contributor

I think it shouldn't conflict with the rule max-attributes-per-line.

I agree with this. So for example, I wouldn't expect this to be invalid:

<div
  class="panel"
>content</div>

Because the attribute is technically on a new line from the content. However @alendorff, I would expect these to be invalid:

<div arg1>content</div>

<div arg1 arg2>content</div>

And autofixed to:

<div arg1>
  content
</div>

<div arg1 arg2>
  content
</div>

Because I don't see how it conflicts with max-attributes-per-line - though apologies if I've missed something. The last example you gave would also be valid:

<div arg1 
     arg2
>
  content
</div>

Does that make sense?

@maksnester
Copy link

maksnester commented Jul 31, 2018

Got it, thanks. Yes, makes sense.

@ota-meshi
Copy link
Member Author

@michalsnik @chrisvfritz

html-tag-attrs-content-new-line

Sounds good to me.

However, I would like to enforce new line in the following html after <tr> and before </ tr>.

<tr><td>cell1</td>
<td>cell2</td>
<td>cell3</td></tr>

I think it is necessary to have a different rule from html-tag-attrs-content-new-line.
That rule enforces a new line after the start tag and before the end tag if the content or children are multiple lines.

@chrisvfritz
Copy link
Contributor

@ota-meshi I agree would be useful! 🙂 Although with this specific proposal:

That rule enforces a new line after the start tag and before the end tag if the content or children are multiple lines.

Then we may have an issue with autofix, because the execution order would become important. For example, with this code:

<tr><td foo>cell1</td></tr>

We'd have to ensure that any other rules that add/remove newlines is run first. In the case of html-tag-attrs-content-new-line, it would make the content multiline where it wasn't before:

<tr><td foo>
  cell1
</td></tr>

Without controlling the execution order, users would have situations where they'd have to ESLint with --fix multiple times to actually fix everything.

@michalsnik Is controlling the execution order of rules something that's feasible or do we need to think of a slightly different rule that doesn't rely on it?

@ota-meshi
Copy link
Member Author

Since eslint verifies up to 10 times until autofix is resolved, I think the possibility of this issue remaining is very low.

https://github.com/eslint/eslint/blob/master/lib/linter.js#L1156

@chrisvfritz
Copy link
Contributor

Since eslint verifies up to 10 times until autofix is resolved, I think the possibility of this issue remaining is very low.

@ota-meshi Oh, cool! I didn't know that. 🙂

@ota-meshi
Copy link
Member Author

Closing to continue working with #551 and #552.

@ota-meshi ota-meshi closed this Aug 11, 2018
@ota-meshi ota-meshi deleted the add-html-content-newlines branch August 13, 2018 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants