vuejs / v2.vuejs.org

๐Ÿ“„ Documentation for Vue 2
https://v2.vuejs.org
MIT License
5.04k stars 3.43k forks source link

Remove Multi-attribute-elements-strongly-recommended #2049

Open posva opened 5 years ago

posva commented 5 years ago

I think we should reconsider https://vuejs.org/v2/style-guide/#Multi-attribute-elements-strongly-recommended and its dedicated eslint rule that is included by default in strongly-recommended It doesn' make sense to split per attribute count as they can be very short leading to extremely narrow-column-looking html. There was this issue https://github.com/vuejs/eslint-plugin-vue/issues/378 but I don't think it makes sense to support both rules I think we should split when the line is above a certain length (like prettier does). And this can already be achieved by deactivating the eslint rule for max-attributes-per-line and using prettier but recommending to split per attribute count is bad. Take vuetify as an example:

          <v-layout
            shrink
            align-center
          >
            <v-btn
              small
              depressed
            >Engagement</v-btn>
            <v-btn
              small
              depressed
              color="primary"
            >
              <v-icon>loop</v-icon> GED
            </v-btn>
            <v-btn
              depressed
              small
              color="info"
            >
              <v-icon>cast</v-icon>
            </v-btn>
            <v-flex class="bold">21H 10 MIN</v-flex>
          </v-layout>
        </v-layout>

This would be a good example:

<a :href="href" class="short" target="_blank">Foo<a>
<a
  foo="a very long multi-tribute element that exceeds the max length"
  any
  other
  attr
  should
  be
  on="its own line"
>

For anybody interested in having the obove formatting working automatically, it's achievable by deactivating the rule vue/max-attributes-per-line in .eslintrc with "vue/max-attributes-per-line": 0 and make sure you don't have "vetur.format.defaultFormatter.html": "js-beautify-html" in your VSCode settings

posva commented 5 years ago

Pinging relevant people @phanan @michalsnik @mysticatea

Justineo commented 5 years ago

What do you think about this?

<a
  foo="a very long multi-tribute element that exceeds the max length"
  any other attr should be on="its own line"
>
posva commented 5 years ago

I wonder if it's feasible, I'm not against but when using prettier you get every single one in a new line. To me that reminds me of:

{
  foo: "a very long multi-tribute element that exceeds the max length",
  any: '', other: '', attr: '', should: '', be: '', on: 'its own line'
}

which I would totally avoid ๐Ÿ˜†

Justineo commented 5 years ago

For a single element, it is indeed easier to read. But to me, splitting each attribute into a new line would make it harder to figure out the overall hierarchy of the document. Though it's what we do for JavaScript objects, we won't encounter such deep structure as often as in templates.

posva commented 5 years ago

yeah, but I think if someone needs that customization they will have to disable the rule altogether and go manually unless it is doable with eslint, in which case an option would allow different possibilities The thing I wanted to talk about is recommending attributes to be on multiple lines just because of the amount of attributes, the eslint rule is a completely different thing and I think it will be worth proposing it afterwards in the eslint plugin vue repo

phanan commented 5 years ago

The reason we use

<a
  foo="a very long multi-tribute element that exceeds the max length"
  any
  other
  attr
  should
  be
  on="its own line"
>

in our codebase is because it's quite simple to reason about (and effective, I'd say): either all attributes are on the same line (if the max length isn't exceeded), or none of them. This applies for HTML, JS, and even backend structs alike.

phanan commented 5 years ago

Pinging @chrisvfritz because he's the original author of the rule.

AllanOcelot commented 5 years ago

I agree with @phanan on this, I prefer being able to read down in a column like style. However I do agree that in many cases, the examples given (of multiple in one line) do make sense.

We should make it more clear to the community that it is a choice. Maybe strongly recommended is a bit too... strong ๐Ÿ˜†

jak-kal commented 5 years ago

For me, I find that splitting attributes into one-per-line decreases the overall readability of my markup. I favour readability and always try and make effective use of whitespace.

I've seen a few of my colleagues start writing this way - as per the docs - and I cannot stand it. Any more thoughts on this?

thedamon commented 4 years ago

When having multiple attributes per line, especially if they have even short javascript expressions in them, it gets very difficult to read and also makes diffs complex and there for commit conflicts more likely and more difficult to resolve.

This is especially true of programmatic attributes (directives). having a v-if, v-model and an event bound in the same line is not gonna be clear. To me I might think that boolean attributes can squish together, but events, directives and bindings really feel like they should be on different lines to me.

As far as the 'readability'. probably largely what you're used to. i have vetur.prettyhtml making my attributes 1 per line

More discussion and a LOT of opinions on why this makes sense as an option here: https://github.com/prettier/prettier/issues/5501

And also a lot of push back from the prettier team who really doesn't want it supported.

Now while I absolutely agree with the formatting choice of 1 attribute per line, it is also surprisingly difficult to config editors/linters/formatters to consistently follow this because as soon as prettier gets involved it stops working. So vue cli's eslint+prettier option doesn't follow the rule, and as far as I can tell neither does the vue-enterprise-boilerplate. That might be a reason to at least downgrade it from strongly-recommended if nearly all vue code built using the 'standard' tools isn't able to follow it.