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

feat: ignore anchor tags in multiline rule #738

Merged
merged 8 commits into from Feb 3, 2019
Merged

feat: ignore anchor tags in multiline rule #738

merged 8 commits into from Feb 3, 2019

Conversation

manniL
Copy link
Contributor

@manniL manniL commented Dec 24, 2018

While applying version 5 of eslint-plugin-vue to my personal page I realized that most of my anchor tags had additional whitespaces. This is the result of multi-attribute anchor tags + the vue/singleline-html-element-content-newline/# vue/multiline-html-element-content-newline rule.

image

image

I'd vote for more sensible defaults (by ignoring a as well) here ☺️

@mysticatea
Copy link
Member

Thank you for this issue.

But I'd like to oppose this change because <a> elements don't display whitespaces, as different to <pre> and <textarea> elements. I don't think that there is a reasonable reason in order to address <a> as special.

@manniL
Copy link
Contributor Author

manniL commented Dec 29, 2018

@mysticatea Thanks for the response!

The problem I suffer is visualized below. Anchor elements inside of other elements like p will honor one trailing whitespace. With the current defaults I can't "remove" the whitespace because a line break is necessary.

image
(CodePen)

@mysticatea
Copy link
Member

Oh, I got it. Thank you for the explanation.

OK, looks good to me. Would you update the documentation of the rule?

@manniL
Copy link
Contributor Author

manniL commented Dec 30, 2018

@mysticatea Will do ☺️

Do you think including nuxt-link/router-link would make sense as well?

@ota-meshi
Copy link
Member

I think that singleline-html-element-content-newline and multiline-html-element-content-newline rules should be turned off if your App is sensitive to whitespace in inline elements. Even if you exclude HTML inline elements, you can not report correctly if the user changes in CSS.

We may need to exclude singleline-html-element-content-newline and multiline-html-element-content-newline rules from config.

singleline-html-element-content-newline and multiline-html-element-content-newline rules are very effective when users want to unify code styles.

@manniL
Copy link
Contributor Author

manniL commented Dec 30, 2018

@ota-meshi Removing them from the defaults would be another "workaround" 🤔

@michalsnik
Copy link
Member

Hi @manniL Nice to see you here, and thanks for the PR :)

What you might also want to consider in your specific scenario is to use preserveWhitespace option in vue-loader: https://vue-loader.vuejs.org/options.html#compileroptions
Though it's being deprecated now in favour of new whitespace option, that just have been merged: vuejs/vue@e1abedb

It will provide condense setting, that in V3 will probably be the default one, making our default configuration in eslint-plugin-vue a bit more justified.

I'm not sure we should ignore a tags by default. You might always opt-out, by updating configuration in your local .eslintrc file.

@manniL
Copy link
Contributor Author

manniL commented Dec 31, 2018

Hey @michalsnik 👋
Long time no see 🙊

I'm looking forward to the condense option but I think it won't solve the problem as <a> aaa </a> or

<a>
  aaa
</a>

will "just" be converted to <a> aaa </a> which still includes the leading and trailing space.

All in all, using a tags with the defaults for html-element-content-newline rules is a bit tedious. And changing the defaults for each project only to have normal link behavior (or use v-text which is my current workaround) shouldn't be necessary IMO ☺️

@michalsnik
Copy link
Member

Hmm, true @manniL 🤔

Another alternative solution might be to set display: inline-block on your a elements.

Though I'm not opposed to consider ignoring a tags, I'd love to hear @chrisvfritz opinion too :)

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Jan 4, 2019

This is the formatting I typically prefer in this case:

<a
  foo="a"
  bar="b"
  baz="c"
>content</a>

And for reference, Prettier will currently format to:

<a
  foo="a"
  bar="b"
  baz="c"
  >content</a
>

Honestly not sure if that behavior is intentional or a bug though. 😄

Either way, I agree that the defaults should preserve whitespace, possibly for all elements since we can't really know an element's true display, if overridden in CSS.

@ota-meshi
Copy link
Member

I checked the list of "inline" elements on Chrome and listed the elements of display: inline;.

Inline elements | MDC | List of "inline" elements

the elements of `display: inline;`
[
  "a",
  "abbr",
  "acronym",
  "audio",
  "b",
  "bdi",
  "bdo",
  "big",
  "br",
  // display: inline-block
  // "button",
  "canvas",
  "cite",
  "code",
  "data",
  // display: none
  // "datalist",
  "del",
  "dfn",
  "em",
  "embed",
  "i",
  "iframe",
  "img",
  // display: inline-block
  // "input",
  "ins",
  "kbd",
  "label",
  "map",
  "mark",
  // display: inline-block
  // "meter",
  "noscript",
  "object",
  "output",
  "picture",
  // display: inline-block
  // "progress",
  "q",
  "ruby",
  "s",
  "samp",
  // display: inline-block
  // "select",
  "small",
  "span",
  "strong",
  "sub",
  "sup",
  "svg",
  // display: inline-block
  // "textarea",
  "time",
  "u",
  "tt",
  "var",
  "video",
  "wbr"
]

We might be able to do something special process using this list.

@michalsnik
Copy link
Member

And excluding void elements + elements not supported in HTML 5 we get the following list:

[
    "a",
    "abbr",
    "audio",
    "b",
    "bdi",
    "bdo",
    "canvas",
    "cite",
    "code",
    "data",
    "del",
    "dfn",
    "em",
    "i",
    "iframe",
    "ins",
    "kbd",
    "label",
    "map",
    "mark",
    "noscript",
    "object",
    "output",
    "picture",
    "q",
    "ruby",
    "s",
    "samp",
    "small",
    "span",
    "strong",
    "sub",
    "sup",
    "svg",
    "time",
    "u",
    "var",
    "video"
]

@michalsnik
Copy link
Member

What do you guys think about current implementation? I ignore all inline elements implicitly now, no matter ignores setting. But I left pre + textarea as is, so one can override these. Should we also allow re-enabling this rule for other tags too?

@manniL
Copy link
Contributor Author

manniL commented Jan 7, 2019

I like the implementation @michalsnik ☺️

Regarding your question: Who would re-enable it for a few tags? Is the need there to do so (and not disabling the whole rule instead)?

@michalsnik
Copy link
Member

Thanks @manniL I meant that one could want for example not to ignore a tags for some reason. And in the current implementation it's not possible as I'm implicitly ignoring it anyway no matter what user passes in ignores option.

@manniL
Copy link
Contributor Author

manniL commented Jan 7, 2019

@michalsnik Hmm, I see. Don't seem like a lot of overhead, so why not 🤷‍♂️ ☺️

@michalsnik
Copy link
Member

On the other hand it might be an overhead for users.
If we allow overriding ignores completely, then if they would want to disable all inline elements but label - they'd have to copy whole list of inline elements except label and paste it in their .eslintrc, or alternatively we might create an extra whitelist option, but I doubt using both ignores and whitelist in one rule is a good solution, it'd be confusing AF :D

Btw. Did you know handlebars had whitespace trimmer built-in {{~ foo ~}}? :D

@manniL
Copy link
Contributor Author

manniL commented Jan 7, 2019

@michalsnik Well, we could just put the very broad default list in ignores and let them change it if they want. There is no way to "satisfy" all options (or make them all easy) 🙈

So if ppl want to ignore all from the list but label, they have to add them all manually. That's more intuitive I guess.

PS: I did not 😄

@michalsnik
Copy link
Member

Fine 👍 thanks @manniL I needed that extra opinion

@ota-meshi
Copy link
Member

ota-meshi commented Jan 8, 2019

Do you think that <pre> and <textarea> should also be ignored implicitly?

html-indent rule implicitly ignores <pre> and <textarea>.

@manniL
Copy link
Contributor Author

manniL commented Jan 20, 2019

@ota-meshi Good point. Yeah, for consistency reasons we should include them IMO.

@michalsnik
Copy link
Member

I simplified the configuration in the last commit as agreed with @manniL: #738 (comment)

@michalsnik michalsnik merged commit 98f7bbd into vuejs:master Feb 3, 2019
@manniL manniL deleted the patch-1 branch February 3, 2019 12:03
@blowsie
Copy link

blowsie commented Feb 12, 2019

Not sure if i'm missing something here, but i tested the following rule within my eslintrc.js

'vue/multiline-html-element-content-newline' : [ 'error', {ignores: ['pre', 'textarea', 'a']}]

The result was a fix in the whitespace, but it introduced a conflict with another rule.
image

@mariusa
Copy link

mariusa commented Dec 29, 2021

Please add to ignore list nuxt-link/router-link (#1749)

@mariusa
Copy link

mariusa commented Sep 21, 2022

@ota-meshi Would you please add to ignore list nuxt-link/router-link (#1749) ? Thanks

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.

None yet

7 participants