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

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

Open
posva opened this issue Mar 8, 2019 · 10 comments
Open

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

posva opened this issue Mar 8, 2019 · 10 comments

Comments

@posva
Copy link
Member

posva commented Mar 8, 2019

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 vuejs/eslint-plugin-vue#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
Copy link
Member Author

posva commented Mar 8, 2019

Pinging relevant people @phanan @michalsnik @mysticatea

@Justineo
Copy link
Member

Justineo commented Mar 8, 2019

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
Copy link
Member Author

posva commented Mar 8, 2019

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
Copy link
Member

Justineo commented Mar 8, 2019

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
Copy link
Member Author

posva commented Mar 8, 2019

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
Copy link
Member

phanan commented Mar 8, 2019

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
Copy link
Member

phanan commented Mar 8, 2019

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

@AllanOcelot
Copy link

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
Copy link

jak-kal commented Sep 4, 2019

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
Copy link

thedamon commented Feb 11, 2020

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: prettier/prettier#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.

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

No branches or pull requests

6 participants