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

Option to never wrap single attributes and always wrap multiple attributes #45

Closed
thedamon opened this issue Oct 18, 2018 · 6 comments
Closed
Labels
enhancement New feature or request proposal

Comments

@thedamon
Copy link

thedamon commented Oct 18, 2018

🚀 Feature Proposal

I don't see a setting for attribute wrapping other than printWidth. This is surprisingly ok since with a pretty low printWidth, the default formatting is quite to my liking, but it ends up looking odd when wrapping single attribute elements. I would love a setting like this:

"wrapAttributesIgnoreSingle": true,
"wrapAttributesNumber": 2 // wrap all attributes when there are n or more 

or more simply but less flexible:

"wrapMultipleAttributes": true

Motivation

Approaching wrapping based on attribute number instead of length is fairly visually appealing, it makes it a little easier for humans to read, but so much easier for git to read, so it will reduce the number of git conflicts if multiple people change an element's attributes. It is also in accordance with the official Vue.js styleguide

Example

Current output:

  <InputItem :input="inputs.currentAge"/>
  <InputItem
    :input="inputs.theRetirementAgeOfYourGreatestGrandMother"
  />
  <a href="#" class="a" rel="tag">A</a>

I would want a combination of settings that would get me this output:

  <InputItem :input="inputs.currentAge"/>
  <InputItem :input="inputs.theRetirementAgeOfYourGreatestGrandMother"/>
  <a 
    href="#" 
    class="a" 
    rel="tag"
  >A</a>

Vote

If you agree with my proposal vote with a 👍

@StarpTech
Copy link
Member

Hi @thedamon thanks for your proposal! I understand your reasons but I'm not convinced from your examples. Wrapping attributes based on the printWidth work for most of the use cases. I don't want to add another option when they aren't really required.

If you want to archive this formatting with the current solution you can use the ignore flag:

<InputItem :input="inputs.currentAge"/>
<!-- prettyhtml-ignore-->
<InputItem :input="inputs.theRetirementAgeOfYourGreatestGrandMother"/>
<!-- prettyhtml-ignore-->
<a href="#" class="a" rel="tag">A</a>

@StarpTech StarpTech added proposal enhancement New feature or request labels Oct 19, 2018
@transtone
Copy link

transtone commented Oct 27, 2018

a option like js-beautify-html's wrap_attributes: force-aligned.

@StarpTech
Copy link
Member

Please have in mind that prettyhtml is a formatter for everyone not only for vue. While I agree that the situation can be tricky in some cases I disagree to add another option in order to be more flexible. The goal with prettyhtml is to provide a consistent and opinionated formatter with less options as possible.

But I don't disagree with your proposal! Let's see how other people think about it. They can vote with a 👍

@thedamon
Copy link
Author

thedamon commented Oct 29, 2018

I don't mean to single out Vue necessarily, but it is worth considering that their formatting recommendation is quite a good one, and also that Vue is called out in the first line of the library description. To explain a little more why I think attribute number makes more sense than character line length for beautifying html:

Javascript is a very flexible, statement driven language, where different things can be written in many different ways and we have no idea what people will write when they open a *.js file. It is much more difficult to mandate that ternaries/ifs/returns/braces etc should split up a certain way all the time and have it look appealing and readable still... but with HTML/XML it is a series of elements that have 0-Infinity attributes and optional content inside. It's much more predictable what it will look like. In that way I think formatting based on the number of attributes is very reasonable. Based on my usage of prettyhtml so far, I also think that it actually looks better with or without any other rationale. elements with one attribute on another line looks weird. multiple attributes always splitting across multiple lines looks better as well as being easier to read.

Pros:

  • Protection from git conflicts when multiple attributes are edited separately
  • Looks better
  • More readable
  • Follows some official framework styleguides

Cons:

  • Adds an option to a tool that is meant to have as few options as possible.

In order to address the con, I'd suggest changing the whole thing to be based on number of attributes instead of line length since as explained i think with HTML it makes more sense to do it that way, but I understand perhaps this is meant to have a similar API to prettier.js and probably most of the library is based around doing it that way.

@StarpTech
Copy link
Member

StarpTech commented Oct 29, 2018

@thedamon thanks for your detail view on the things. Initially, I added attribute wrapping based on printWidth due to the fact that it allows optimizing the content according to the available space.

This is the common approach for almost any markup formatter and it works in a wild. If we would wrap based on attributes count we could end up like this:

Initial

<div a="" b=""  c=="" d="" ></div>
<div a="" b=""></div>

Based on attribute wrapping:

  • What's the magical number of attributes? The simplest solution would be to wrap at 2 attributes.
<div
	a=""
	b="" 
	c==""
	d=""></div>
<div
	a=""
	b=""></div>

What I don't like is that we will waste a lot of space and the height of your page will increase drastically.

Here are some more real world examples:

Initial: Example

PrintWidth: 80

<div class="article-meta">
  <router-link
    :to="{ name: 'profile', params: { 'username': article.author.username } }"
  >
    <img :src="article.author.image">
  </router-link>
  <div class="info">
    <router-link
      :to="{ name: 'profile', params: { 'username': article.author.username } }"
      class="author"
    >{{ article.author.username }}</router-link>
    <span class="date">{{ article.createdAt | date }}</span>
  </div>
  <template v-if="actions">
    <rwv-article-actions :article="article" :canModify="isCurrentUser()"></rwv-article-actions>
  </template>
  <template v-else>
    <button
      class="btn btn-sm pull-xs-right"
      v-if="!actions"
      v-on:click="toggleFavorite"
      :class="{
        'btn-primary': article.favorited,
        'btn-outline-primary': !article.favorited
        }"
    >
      <i class="ion-heart"></i>
      <span class="counter">{{ article.favoritesCount }}</span>
    </button>
  </template>
</div>

Based on attribute wrapping:

<div class="article-meta">
  <router-link :to="{ name: 'profile', params: { 'username': article.author.username } }">
    <img:src="article.author.image">
  </router-link>
  <div class="info">
    <router-link
      :to="{ name: 'profile', params: { 'username': article.author.username } }"
      class="author"
    >{{ article.author.username }}</router-link>
    <span class="date">{{ article.createdAt | date }}</span>
  </div>
  <template v-if="actions">
    <rwv-article-actions
      :article="article"
      :canModify="isCurrentUser()"
    ></rwv-article-actions>
  </template>
  <template v-else>
    <button
      class="btn btn-sm pull-xs-right"
      v-if="!actions"
      v-on:click="toggleFavorite"
      :class="{
        'btn-primary': article.favorited,
        'btn-outline-primary': !article.favorited
        }"
    >
      <i class="ion-heart"></i>
      <span class="counter">{{ article.favoritesCount }}</span>
    </button>
  </template>
</div>

Cons:

  • Very long single attributes aren't wrapped on a newline.
  • You will lost much of space in both width and height.
  • Your page height will increase because many newlines are added. In other words it doesn't scale. Minitors getting bigger in width not height.

Protection from git conflicts when multiple attributes are edited separately

That's true but in my experience this wasn't a big deal or issue.

Looks better
More readable
Follows some official framework styleguides

This is very subjective and I don't know another framework except vue.

@StarpTech
Copy link
Member

I decided to add a new option called wrapAttributes when true it will always wrap attributes when the element has more than one.

StarpTech added a commit that referenced this issue Nov 8, 2018
* add wrapAttributes option

* add wrapAttributes option

* add wrapAttributes option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request proposal
Projects
None yet
Development

No branches or pull requests

3 participants