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

Enforce Single Attribute Per Line (#5501) #6644

Merged

Conversation

kankje
Copy link
Contributor

@kankje kankje commented Oct 13, 2019

Added the htmlSingleAttributePerLine option for specifying if Prettier should enforce single attribute per line in HTML. Fixes #5501.

The implementation is based on this comment: #5501 (comment).

I checked out the option philosophy and it seems like this shouldn't be an option but I figured that the option would be better/safer for backward compatibility. I'd appreciate feedback on this though.

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@lydell
Copy link
Member

lydell commented Oct 14, 2019

If we add an option, shouldn’t it also affect Vue and JSX?

@alexander-akait
Copy link
Member

I think if we add option we should respect this option for all html like syntax

@kankje
Copy link
Contributor Author

kankje commented Oct 14, 2019

Hi, the option supports Vue and JSX the same way the htmlWhitespaceSensitivity option does. Should the option be renamed?

@lydell
Copy link
Member

lydell commented Oct 14, 2019

htmlWhitespaceSensitivity does not affect JSX. But you’re right that it does affect Vue.

Maybe singleAttributePerLine?

I only see tests for HTML?

@kankje
Copy link
Contributor Author

kankje commented Oct 14, 2019

I added support for JSX and added tests for both Vue and JSX.

@kankje kankje changed the title HTML: Enforce Single Attribute Per Line (#5501) Enforce Single Attribute Per Line (#5501) Oct 14, 2019
@fisker
Copy link
Member

fisker commented Oct 15, 2019

I thought we don't accept those kind of options, remember how long takes to add --vue-indent-script-and-style ? #3888

@lydell
Copy link
Member

lydell commented Oct 15, 2019

We haven’t decided what to do yet. But if @kankje bothered to make a PR would could just as well make it nice. I think it helps discussion.

@kankje
Copy link
Contributor Author

kankje commented Oct 17, 2019

I'm fine with making this the default behavior if you feel like that's the way to do it.

@yippie
Copy link

yippie commented Oct 17, 2019

Yes please, I think this is the way to go by default

@CaptainYouz

This comment has been minimized.

@kankje
Copy link
Contributor Author

kankje commented Oct 28, 2019

@lydell What do you think? Should I change this to be the default behavior? I'm fine either way, I'd just like to get this reviewed/merged as I think this change will result in more readable code.

@lydell
Copy link
Member

lydell commented Oct 28, 2019

@kankje This PR is good as-is. However, this will most likely stay open for quite a while as we decide what to do. First, we need to focus on getting 1.19 out.

#3101 and #5501 combined have the most 👍s of all issues and is one of the least bike-sheddy option requests. So if we’re going to add another option, this one for sure has potential.

@thorn0 thorn0 added the type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity. label Oct 31, 2019
@JounQin
Copy link
Member

JounQin commented Nov 16, 2019

Why not keep it like literal object?

If the developer write the first attribute in a new line, we should keep one attribute per line in the following, if he doesn't, we should keep it as is.

Don't add such option please, we "hate" options.

@SFWNSFW

This comment has been minimized.

@lydell

This comment has been minimized.

@dennishall
Copy link

dennishall commented Feb 21, 2020

Why not keep it like literal object?

If the developer write the first attribute in a new line, we should keep one attribute per line in the following, if he doesn't, we should keep it as is.

Don't add such option please, we "hate" options.

--> because then it's not a unified rule, it's a segmented rule that behaves differently depending on the individual developer --- exactly what we're trying to avoid with tools like this. However, I'm not opposed to making it a flavor of this rule: maxAttributesPerLine: 1 OR wrapAttributes: [many options, including "object-literal"] -- see js-beautify's wrap-attributes for example. (https://github.com/beautify-web/js-beautify#css--html)

@miszo
Copy link

miszo commented Feb 24, 2020

Hi, any news regarding this PR? I really can't wait for it to be included in Prettier. @kankje, did you consider publishing this feature as a plugin until Prettier Team decide what to do with it?

@maogongzi
Copy link

@miszo Feel free to try out my fork at https://github.com/maogongzi/prettier/tarball/patch-single-line-attr-v0.1 ,simply replace the prettier version from your package.json to the following:

"prettier": "https://github.com/maogongzi/prettier/tarball/patch-single-line-attr-v0.1",

@diachedelic
Copy link

@maogongzi I have installed your fork but it does not seem to force attributes onto their own line (fill and d should be on separate lines)

$ npx prettier src/icons/Add.vue 
<template>
  <svg
    class="xc-icon xc-add"
    viewBox="2 2 20 20"
    xmlns="http://www.w3.org/2000/svg"
  >
    <path fill="none" d="M0 0h24v24H0V0z" />
  </svg>
</template>

$ npx prettier --version
1.20.0-dev

// yarn.lock
prettier@^1.18.2, "prettier@https://github.com/maogongzi/prettier/tarball/patch-single-line-attr-v0.1":
  version "1.20.0-dev"
  resolved "https://github.com/maogongzi/prettier/tarball/patch-single-line-attr-v0.1#56db4cff79930f505538c4ba25507a92727b82a6"

@maogongzi
Copy link

@diachedelic Yes I understand, that's perhaps your .eslintrc needs some tiny tweaks, it really takes skill to get Vue, ESlint and Prettier tuned, I'll send you a copy of my .prettierrc, .eslintrc, and .editorconfig so that you can compare with your own config to figure out what's happening, for me, the svg you pasted out is formatted like below, each attr stays on it's own line:

      <!-- dialog title -->
      <div class="dialog__header clearfix">
        <svg
          class="xc-icon xc-add"
          viewBox="2 2 20 20"
          xmlns="http://www.w3.org/2000/svg"
        >
          <path
            fill="none"
            d="M0 0h24v24H0V0z"
          ></path>
        </svg>
        <h3>
          <template v-if="mode === 'create'">

My config files pasted here:

.editorconfig
https://pastebin.com/JBPD4UeC

.prettierrc
https://pastebin.com/BnkzDAsm

.eslintrc
https://pastebin.com/8iSHG4DW

Have a good day!

@diachedelic
Copy link

diachedelic commented Feb 28, 2020 via email

@NoPhaseNoKill
Copy link

NoPhaseNoKill commented Mar 15, 2020

Are we able to merge this? I'm hanging out for this to be added

edit: Does anyone know of a way where I can get the desired output (ie only one prop per line) without this? Am I able to configure my eslint lint alongside prettier somehow?

edi2: I do not want eslint errors in my IDE, I just want prettier to somehow use the eslint rules (in this case 1 prop per line) I set when it runs during pre-commit. This is not only for this rule, but others too.

E.g.

No error in IDE using this:

<Button name='test' id='123' someRandomProp={false}/>

But when I use my prettier pre-commit hook I want the output file to be:

<Button name='test' 
        id='123' 
        someRandomProp={false}
/>

@j-f1

This comment has been minimized.

@NoPhaseNoKill

This comment has been minimized.

@NoPhaseNoKill

This comment has been minimized.

@fisker
Copy link
Member

fisker commented Nov 29, 2021

I'm fine adding this option, though I won't use it myself.

@jodaka
Copy link

jodaka commented Dec 2, 2021

So who's gonna be that Hero who pushes Merge button?

@alexander-akait alexander-akait merged commit 0e42acb into prettier:main Dec 2, 2021
@alexander-akait
Copy link
Member

I hope I didn't offend anyone by doing this 😨

@egorovsa
Copy link

egorovsa commented Dec 2, 2021

My goodness. It happened! Thanks !

@alexander-akait
Copy link
Member

We took too long to decide on this, it's time to move forward, this is a fairly popular request that I think we should satisfy

@liquidvisual
Copy link

I don't see the new option in the docs?

sosukesuzuki added a commit that referenced this pull request Dec 2, 2021
@miszo
Copy link

miszo commented Dec 2, 2021

I don't see the new option in the docs?

@liquidvisual
I guess that it's not released yet.

@miszo
Copy link

miszo commented Dec 2, 2021

@sosukesuzuki,

This is disturbing: 44bffcd

So, you're going to revert it? Or it is just for some release purposes and it will be re-reverted?

@sosukesuzuki
Copy link
Member

@miszo To release new patch version. Please read the description of #11906. I'm sorry for making you uncomfortable.

@miszo
Copy link

miszo commented Dec 2, 2021

@sosukesuzuki,

No worries 🙂
Maybe that was wrong wording from my side - I was more confused than disturbed 😅

I understand - thanks for the clarification anyways and good luck with releasing patch version! 🤞

@fisker
Copy link
Member

fisker commented Dec 3, 2021

I didn't see a changelog, and it says _First available in v2.3.3_.

@fisker
Copy link
Member

fisker commented Dec 3, 2021

@kankje ^

@kankje
Copy link
Contributor Author

kankje commented Dec 5, 2021

I didn't see a changelog, and it says _First available in v2.3.3_.

@fisker What would you like me to do? Should I open a new PR that updates the "first available in" version to 2.6.0 and adds a changelog to changelog_unreleased/html?

@fisker
Copy link
Member

fisker commented Dec 6, 2021

Yes, please

@MilesHeise
Copy link

MilesHeise commented Dec 7, 2021

I see this getting added and then reverted (and a big convo about version numbers in the linked revert PR) . . . is there a best place to check/subscribe to find out when this feature is actually available? for accessibility reasons my team is totally blocked on adding Prettier to HTML without this option so I have been following this issue eagerly for a couple years now and definitely psyched to see it might finally be coming :-D

definitely want to upgrade versions the second this makes it in . . .

@scherii
Copy link

scherii commented Dec 7, 2021

Hi @MilesHeise

I suggest you keep an eye out for the blog post for version 2.6:
https://prettier.io/blog/

Now that the work is done, it's just wait and see.
And rest assured that I am at least as eager for this release. 😉

You can install the dev version of 2.6.0 with:

  • npm install --save-dev prettier/prettier
  • yarn add -D prettier/prettier

And add the option from the documentation:
https://prettier.io/docs/en/next/options.html#single-attribute-per-line

@MilesHeise
Copy link

excellent, thanks for those details!

@Austaras Austaras mentioned this pull request Dec 15, 2021
4 tasks
@sosukesuzuki sosukesuzuki mentioned this pull request Mar 13, 2022
4 tasks
@sosukesuzuki
Copy link
Member

This has been released! https://prettier.io/blog/2022/03/16/2.6.0.html

@chriszrc
Copy link

chriszrc commented Aug 8, 2022

Was there ever a discussion of being able to automatically alphabetize the attributes as well? I really want this :)

@JounQin
Copy link
Member

JounQin commented Aug 8, 2022

Was there ever a discussion of being able to automatically alphabetize the attributes as well? I really want this :)

It is out of scope of prettier core itself.

https://prettier.io/docs/en/rationale.html#what-prettier-is-_not_-concerned-about

You can try to implement a prettier plugin by yourself.

medikoo pushed a commit to medikoo/prettier-elastic that referenced this pull request Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change HTML/JSX formatting to have one attribute/prop per line