Multiple classes with class:x,y,z={condition}

[marceloadsj]

Related to: #3376

This PR add a new feature described on the issue above. Basically:

It accepts multiple classes in a single expression, separated by comma ,.
However, the shorthand syntax only, e.g. class:active,primary, cannot be used on this case.

<!-- Toggle both active and primary classes -->
<div class:active,primary="{isActive}">...</div>

---

This PR also fixes a small issue on SSR, because it was really coupled and a bit related to the feature.
If you want me to try split in two PRs, let me know! :)

Check the code below, and the SSR generated on compilation time:

<!-- Simple component example -->
<div
  {...$$props}
  class="one"
  class:two="{value === 123}"
  class:three="{value === 123}"
></div>

// Code generated on SSR
`<div${index.spread([
  $$props,
  {
      class: [`one`, `${value === 123 ? "three" : ""}`].join(" ").trim() } // just the last class: expression here
])}${index.add_classes(
  [value === 123 ? "two" : "", value === 123 ? "three" : ""].join(" ").trim() // all class: expressions here
)}></div>`;

Note that on first part of SSR code, it just inject the last class: expression, and on add_classes function, all class: expressions are injected. This generate the issue below:

<!-- Expected -->
<div class="one two three"></div>

<!-- Received on v3.8.1 -->
<div class="one three" class="two three"></div>

Now, with this PR, the code generated on SSR is:

`<div${index.spread([
  $$props,
  {
    class: [
      `one`,
      `${(value === 123 ? "two " : "") + (value === 123 ? "three " : "")}`
    ]
      .join(" ")
      .trim()
  }
])}></div>`;

Maybe we will need to change a bit yet to generate a clean array of classes, but, for now, I chose the easiest way, only changing a few lines of code.

[marceloadsj]

Guys, I'm just renaming this to WIP because I found an issue related to the feature we should think about:

<div class:one,two="{true}" class:three,one="{false}">Hello World</div>

The first class: statement will inject the classes "one", and "two" cause it's true.
Then the second statement will remove classes "three" and "one" cause it's false.

You will end with <div class="two"> only, that's wrong result.

Today, we already have this problem with the following code:

<div class="one two" class:one="{false}"></div>

But, because the feature focus on an easier way to inject multiple classes, we should fix this to not end with a broken feature from the beginning. 😂

[marceloadsj]

I think I have a proposal to these problems:

First, it's not a problem with Svelte, but with the logic itself. Make sense the class to be injected and, after, removed. This is what this code do: <div class="one two" class:one="{false}"></div>

Then, what we can improve is the way we handle this on the compiler.

1. Treat class= as high priority over the others, so we can end up always with the classes on the class= attribute;

2. Throw an error or warning when we found some useless attribute, like on that code:

<div class="one two" class:one="{condition1}"></div>
<!-- the classes one and two will always be injected, so the class:one is useless, don't matter the condition -->

3. On the multiple classes feature, we throw an error or warning and suggest to move the class to a separate attribute:

<div
  class:one,two,three="{condition1}"
  class:four,five,one="{condition2}">
  Hello World
</div>
<!-- the developer should move the class one to a separated attribute, and choose between || or && or whatever he wants -->

<div
  class:two,three="{condition1}"
  class:four,five="{condition2}"
  class:one="{condition1 || condition2}">
  Hello World
</div>

* If both steps 2 and 3 throws an error, the step 1 don't need to be implemented, otherwise we can move the "class=" attribute to the end of the classes logic, to be the topmost priority. In a warning only scenario, we even need to check how to handle the toggle of "class:" attributes, to skip "class=". All of that only if the step 1 was accepted!

What do you think? The steps 1 and 2 can be implemented even without the new feature. Maybe it can be a breaking change to some code relying on this "problem".

Obs: Maybe, on the easiest scenario, we can just put a warning message on the docs or tutorial...

[codecov-io]

Codecov Report

Merging #3419 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3419   +/-   ##
=======================================
  Coverage   50.25%   50.25%           
=======================================
  Files           1        1           
  Lines         197      197           
=======================================
  Hits           99       99           
  Misses         98       98

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05cf649...9d424fe. Read the comment docs.

[marceloadsj]

Guys, I added a new validation function that just warns if we have the same class name in two different class expressions, like:

<div class="one" class:one/>

{
  "code": "class-name-multiple-attrs",
  "message": "Class: avoid using class name 'one' in more than one class attribute"
}

And I added on docs why this is not a good approach!

[parker-codes]

@marceloadsj I love the thought put into this. I use TailwindJS and would love a feature like what you are working on so that I can conditionally apply multiple classes. I also see that the validation can catch some otherwise tricky logical errors. 👍

[jerriclynsjohn]

This is a boon to tailwind users!

[Rich-Harris]

Thank you, and apologies for taking so long to get round to reviewing. There were some fairly substantial changes to the codebase in the meantime, and I've done my best to merge master into this branch, but there's one test — a new test, brought in by #3792 — that isn't passing, and unfortunately I've run out of time to continue working on it.

I can take another run at it another time, unless the solution is obvious to you!

[marceloadsj]

Hey mate, don't worry.

We had a discussion on #3376 about this feature, and didn't agreed on a final API.

Can you helps us with a decision for that?

After this, I can update and merge everything!

[marceloadsj]

Just an update to you guys. Sorry my delay to work on that one, I had some personal problems happening the end of the year.

I will try to make it in the next days. Thank you!

[marceloadsj]

Closing because that solution is deprecated:
Another WIP PR is opened: #4349