Smarter formatting of HTML class attribute #7865

thorn0 · 2020-03-24T21:51:54Z

Break classes onto multiple lines, but try keeping consecutive classes with the same prefix on one line.

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 changelog_unreleased/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
I’ve read the contributing guidelines.

thorn0 · 2020-03-24T21:54:43Z

tests/html_attributes/__snapshots__/jsfmt.spec.js.snap

+    foo
+    bar
+    foo
+    bar


Yes, this looks not exactly good, but this is not a real-life example.

Can be real

Code from this github PR page

Seems like there is no good way to format this real example. Probably we should simply retain new lines in the class attribute, only re-indenting them if needed. That would be pity, the idea with prefixes looked good.

(from my perspective at least, the output in @fisker 's playground is great)

thorn0 · 2020-03-24T21:55:57Z

tests/html_tags/__snapshots__/jsfmt.spec.js.snap

-    class="foo"
+    class="
+      foo
+    "


Note that this is a printWidth: 1 test.

changelog_unreleased/html/pr-7865.md

j-f1 · 2020-03-25T20:50:32Z

src/language-html/syntax-attribute.js

+function getClassPrefix(className) {
+  let idx = className.search(/--|__/);
+  if (idx === -1) {
+    idx = className.indexOf("_");


What about classes with a leading _?

alexander-akait · 2020-04-13T14:17:26Z

changelog_unreleased/html/pr-7865.md

+    flex-column flex-lg-row
+    justify-content-start justify-content-lg-between
+    align-items-start align-items-lg-center
+  "


I'm afraid we will get issues with requests to format each class on each line/don't touch, maybe we put information why we prefer it to docs?

I'm afraid we will get issues with requests to format each class on each line/don't touch, maybe we put information why we prefer it to docs?

For me I prefer one name per line, also consider the following example:

<div class="custom-btn my-very-v-button--mini site-btn my-very-v-button--primary my-very-v-button--ghost">my button</div>

After running prettier, I got:

<div class=" custom-btn my-very-v-button--mini site-btn my-very-v-button--primary my-very-v-button--ghost " > my button </div>

Given 80 as print width, the 3 names starting with my-very-v-button are actually able to fit into one line, but they are not, also it looks better to at least put them next to each other since they share the same prefix, i.e. switch the site-btn line with the last line.

So, my suggestion is let's don't group names with same prefix into one line, just let each name stands in it's own line(which is good for Git diff also), anyway an element or component that has more than 10 class names is really edge case, in that case I should consider my way of using class names, there must be some parts that can be improved to avoid too many class names on the same element.

@maogongzi

the 3 names starting with my-very-v-button are actually able to fit into one line, but they are not

Right, because Prettier never reorders anything. However, one of the points of the current proposal is that in real projects, classes usually are already sorted by prefix, and when they are not, it's for a reason.

@evilebottnawi

I'm afraid we will get issues with requests to format each class on each line/don't touch, maybe we put information why we prefer it to docs?

I added an explanation to the changelog entry. Do you think we also need to write something in the docs about this?

fisker · 2020-04-24T09:51:36Z

All classnames in this PR page

…ve classes with the same prefix on one line

Co-Authored-By: Jed Fox <git@twopointzero.us>

maogongzi · 2020-08-18T07:42:28Z

Any progress on this PR? Do we have a plan to merge it sometime? Up to now it's the only issue blocks me from upgrading Prettier to higher version.

brody4hire · 2020-11-25T20:40:25Z

bump

/cc @pburkindine

…l-classes

thorn0 · 2021-01-18T13:50:42Z

Despite the low interest from the community let's give this a go...

alexcroox · 2021-05-10T08:14:13Z

Is there an easy way to disable this for classes? I've just upgraded and it's making my html templates pretty unreadable when using tailwind vs a wider single line of classes:

ismailcherri · 2021-05-10T10:40:06Z

This is going to be a nightmare for Tailwind CSS users

sod · 2021-05-10T11:25:08Z

I like the idea 👍. Given that we would never write 20 css instructions into a single line in a css file, I don't know how we accepted "minifying" the class attribute to begin with. Yes, it is verbose. But 20 class names is also very complex. I love the idea of grouping classes by prefix.

slauzinho · 2021-05-10T12:44:14Z

Is there a way of disabling this?

thorn0 · 2021-05-10T16:24:02Z

It's not configurable. If #10803 gets implemented, it'll be possible to make a plugin to override this behavior.

TomoyaKuroda · 2021-05-12T16:15:57Z

We should have the option to disable this feature... this is still useful for some users though.

cerpinn · 2021-05-15T13:50:46Z

This is being really annoying, disabling prettier for now.

P.S. I use TailwindCSS

bodograumann · 2021-05-17T07:26:33Z

Yeah, this is not working for us and the tailwind css classes.

First I thought I could maybe refactor the problematic instances to have fewer classes, but that doesn’t work either.
Having them inline is much easier to understand, than artificially refactoring into a class which is not actually reused.

My colleague is already giving me a hard time for automatically formatting with 80 character lines. 😞
Guess I’ll have to stay with prettier v2.2. It’s a pitty because I liked the other changes...

KaragiannidesAgapios · 2021-05-17T09:29:39Z

I'm going to downgrade to 2.2 for now.

I just want to add another problem that this feature introduces.
In my project we use vue test utils for testing.
Problem is that if you test for a classname existence:
expect(button.classes()).toContain('pointer-events-auto')

you get this result which evaluates to false:

Expected value: "pointer-events-auto"
    Received array: ["
    ", "", "", "", "", "", "", "", "", "", "pointer-events-auto
    ", "", "", "", "", "", "", "", "", "", "focus:outline-none
    ", "", "", "", "", "", "", "", "", "", "flex
    ", "", "", "", "", "", "", "", "", "", "items-center
    ", "", "", "", "", "", "", "", "", "", "text-gray-800
    ", "", "", "", "", "", "", "", ""]

Also if you try a workaround your test is not passing again:
expect(button.classes('pointer-events-auto')).toBe(true)
Console logging classes() you get the following result:

    [
      '\n',     '',                      '',
      '',       '',                      '',
      '',       '',                      '',
      '',       'pointer-events-auto\n', '',
      '',       '',                      '',
      '',       '',                      '',
      '',       '',                      'focus:outline-none\n',
      '',       '',                      '',
      '',       '',                      '',
      '',       '',                      '',
      'flex\n', '',                      '',
      '',       '',                      '',
      '',       '',                      '',
      '',       'items-center\n',        '',
      '',       '',                      '',
      '',       '',                      '',
      '',       '',                      'text-gray-800\n',
      '',       '',                      '',
      '',       '',                      '',
      '',       ''
    ]

This is the main reason that i will downgrade.
Indeed it's not optimal imo for tailwind users(i'm one) and it would be nice if this feature would be configurable.

Apart from that, thank you for your effort and have a nice day.

j-f1 · 2021-05-19T12:47:49Z

That looks like a bug in vue-test-utils, @KaragiannidesAgapios. Specifically, this line should change to split on whitespace:

-let classes = classAttribute ? classAttribute.split(' ') : []
+let classes = classAttribute ? classAttribute.split(/\s+/gm) : []

LeoniePhiline · 2021-05-19T19:24:49Z

As a tailwind user, I would need this feature to be smarter. There simply aren't enough prefixes to be grouped. I would want classes to be grouped by domain (positioning, flexbox, spacing, ...), with one line per group.
This sounds like a job for a plugin, because semantic information is necessary.

Otherwise (as second best option), I'd really just want prettier to leave my classes alone and respect the newlines I add for my own grouping. Auto-indenting these groups is of course desired.

alexander-akait · 2021-05-19T19:27:11Z

Please open an issue with example(s) where is bad, no need to write here, it was merged

thorn0 force-pushed the smart-html-classes branch from 13a4c68 to a82a950 Compare March 24, 2020 21:52

thorn0 commented Mar 24, 2020

View reviewed changes

tests/html_tags/__snapshots__/jsfmt.spec.js.snap Outdated

class="foo"

class="

foo

"

Copy link

Member Author

thorn0 Mar 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is a printWidth: 1 test.

thorn0 force-pushed the smart-html-classes branch 2 times, most recently from 0f30eae to cd0956e Compare March 24, 2020 22:25

sosukesuzuki approved these changes Mar 25, 2020

View reviewed changes

j-f1 reviewed Mar 25, 2020

View reviewed changes

thorn0 requested review from j-f1, lipis and alexander-akait April 13, 2020 14:00

alexander-akait reviewed Apr 13, 2020

View reviewed changes

This was referenced Apr 28, 2020

Preserve line breaks in attribute values #8179

Closed

P2.x - Group HTML classes by prefix instead of forcing onto one line #7863

Open

thorn0 force-pushed the smart-html-classes branch from 5877f1c to 272c88c Compare May 26, 2020 17:24

thorn0 and others added 8 commits June 5, 2020 12:30

html: break class attr onto multiple lines, but try keeping consecuti…

ed4432d

…ve classes with the same prefix on one line

add changelog

5e3545e

add more tests

6fedf57

improve doc structure to fix an issue with the closing quote

30c74a3

Update changelog_unreleased/html/pr-7865.md

e3384b8

Co-Authored-By: Jed Fox <git@twopointzero.us>

handle classes with leading _ and -

fa3cffa

update snapshots

379cc43

Lint

338bcc2

thorn0 force-pushed the smart-html-classes branch from 272c88c to 338bcc2 Compare June 5, 2020 18:40

Update pr-7865.md

48605ab

brody4hire mentioned this pull request Aug 19, 2020

HTML class attribute formatting option brody4hire/prettierx#261

Open

Merge branch 'master' into smart-html-classes

2ad5252

lipis approved these changes Dec 12, 2020

View reviewed changes

thorn0 added 2 commits January 16, 2021 16:40

Merge remote-tracking branch 'remotes/upstream/master' into smart-htm…

e3906d7

…l-classes

Update changelog entry

3965aa5

thorn0 merged commit 7cf524f into prettier:master Jan 18, 2021

thorn0 deleted the smart-html-classes branch January 18, 2021 13:55

thorn0 mentioned this pull request Jan 26, 2021

Handlebars: fix formatting of attributes #10145

Merged

3 tasks

kirbysayshi mentioned this pull request Apr 8, 2021

JSX className attribute does not wrap like HTML class attribute #10663

Open

prettier locked as resolved and limited conversation to collaborators May 19, 2021

Smarter formatting of HTML class attribute #7865

Smarter formatting of HTML class attribute #7865

Conversation

thorn0 commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thorn0 Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker commented Apr 24, 2020

Uh oh!

maogongzi commented Aug 18, 2020

Uh oh!

brody4hire commented Nov 25, 2020

Uh oh!

thorn0 commented Jan 18, 2021

Uh oh!

alexcroox commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ismailcherri commented May 10, 2021

Uh oh!

sod commented May 10, 2021

Uh oh!

slauzinho commented May 10, 2021

Uh oh!

thorn0 commented May 10, 2021

Uh oh!

TomoyaKuroda commented May 12, 2021

Uh oh!

cerpinn commented May 15, 2021

Uh oh!

bodograumann commented May 17, 2021

Uh oh!

KaragiannidesAgapios commented May 17, 2021

Uh oh!

j-f1 commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LeoniePhiline commented May 19, 2021

Uh oh!

alexander-akait commented May 19, 2021

Uh oh!

thorn0 commented Mar 24, 2020 •

edited

Loading

fisker Mar 25, 2020 •

edited

Loading

thorn0 Jun 10, 2020 •

edited

Loading

alexcroox commented May 10, 2021 •

edited

Loading

j-f1 commented May 19, 2021 •

edited

Loading