sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.93k stars 4.15k forks source link

Intergrate clsx-Functionality in class-Attribute #9044

Closed AndreasFaust closed 1 year ago

AndreasFaust commented 1 year ago

Describe the problem

As a frequent Tailwind-user I need to handle lots of class-names. To make this convenient and readable I always need the clsx-library: https://github.com/lukeed/clsx

For example:

<script>
import clsx from "clsx"
export let isBlack = false;
</script>

<header
    class={clsx(
        "relative h-min landscape:lg:h-screen",
        "mask-linear mask-dir-to-b",
        "mask-point-from-[80%] mask-to-0",
        { "bg-black": isBlack },
    )}
/>

It would be great to have string-concatenation and object-evaluation integrated in class.

Describe the proposed solution

Integrating clsx-functionality in class. It could be written as a simple array:

<header
    class={[
        "relative h-min landscape:lg:h-screen",
        "mask-linear mask-dir-to-b",
        "mask-point-from-[80%] mask-to-0"
        { "bg-black": isBlack },
    ]}
/>

Alternatives considered

There are already some improvements to the regular class-attribute. Why not go a tiny bit further and make clsx obsolete. Besides that I don’t see an alternative, except keeping it the way it is.

Importance

would make my life easier

brunnerh commented 1 year ago

To me, that example does not really justify changing anything.

It can be written like this without adding any additional features:

<header
    class="relative h-min landscape:lg:h-screen
        mask-linear mask-dir-to-b
        mask-point-from-[80%] mask-to-0"
    class:bg-black={isBlack} />

Granted, it does get more verbose if there are many conditional classes. Also, class:name syntax cannot be used with components.

AndreasFaust commented 1 year ago

Thank you for replying so quickly! Writing the string the way you do is (at least for me using SveltKit and Typescript) not accepted by the Linter. Maybe my example did not demonstrate it well enough, but grouping classes in multiple lines and concatenating strings of classnames is for me as Tailwind-user absolutely crucial to handle the sheer amount of classnames.

Without clsx it would look like this:

<div
    class="absolute z-20 inset-0 object-cover object-center w-full h-full opacity-50 group-hover:opacity-100 transition transition-custom border border-red border-10 {presentVideo ? '!opacity-0 pointer-events-none delay-200 duration-1000' : ''} {loadVideo ? 'opacity-100' : ''}"
/>

With clsx the same example looks much clearer:

<div
    class={clsx(
        "absolute z-20 inset-0 object-cover object-center w-full h-full",
        "opacity-50 group-hover:opacity-100 transition transition-custom",
        "border border-red border-10",
        {
            "opacity-100": loadVideo,
            "!opacity-0 pointer-events-none delay-200 duration-1000": presentVideo,
        }
    )}
/>

Ideally I could write it just like this:

<div
    class={[
        "absolute z-20 inset-0 object-cover object-center w-full h-full",
        "opacity-50 group-hover:opacity-100 transition transition-custom",
        "border border-red border-10",
        {
            "opacity-100": loadVideo,
            "!opacity-0 pointer-events-none delay-200 duration-1000": presentVideo,
        }
    ]}
/>

I hope, this better points out the problem.

brunnerh commented 1 year ago

Writing the string the way you do is (at least for me using SveltKit and Typescript) not accepted by the Linter.

You might want to get a better Linter or ignore whatever rule is responsible, then 😅

You can also just concatenate strings which might pass and is fairly straightforward:

<header
    class={'relative h-min landscape:lg:h-screen ' +
        'mask-linear mask-dir-to-b ' +
        'mask-point-from-[80%] mask-to-0'}
    class:bg-black={isBlack} />

What you suggest here with a component is not going to work. class is just some component property, the compiler does not know how it is used by the component, it could be something completely different. Hence it would potentially lead to issues to introduce magic just based on property names. Even for elements, I for one would be opposed to changing the way certain attributes are handled; things like class:... extend the functionality around classes without messing with the class attribute itself. (Unfortunately the handling of classes when passed to components is not very flexible at the moment. Not sure if there have been proposals to remedy this.)

Also, there already is the following issue requesting the ability to add multiple classes at once based on one condition:

AndreasFaust commented 1 year ago

You are right, that my example would not work with a component — this was also not my intention, just a plain mistake! I, of course, meant simple html-tags. I corrected my examples from the previous post to avoid further misunderstandings. (By the way: I am also not a fan of magically passing class as a kind of special property, as you find it in Vue for example.)

That said: I don’t think that your solutions live up to my array-proposal. There are two challenges:

  1. sorting multiple classnames (into multiple strings) to make them conveniently readable
  2. having multiple classnames bound to a single condition

The second challenge could be almost satisfyingly solved like it was proposed in this example:

<button
    on:click
    class="px-3 py-2 text-white rounded shadow-lg focus:outline-none focus:ring-2 focus:ring-offset-2"
    class:"bg-blue-700 hover:bg-blue-800 ring-blue-400"={color === 'primary'}
    class:"bg-red-600 hover:bg-red-700 ring-red-500"={color === 'danger'}
>
    <slot />
</button>

But the array-syntax is still cleaner.

For the first challenge I see no solution, yet. Getting a "better Linter" or ignoring it is needlesly polemic. As I wrote: I use the default linter of SvelteKit and my aim is, to make reasonable proposals to improve these already great defaults. Concatenating regular strings does not look as tidy and has the downside of appending empty spaces which is error prone:

<header
    class={'relative h-min landscape:lg:h-screen ' +
        'mask-linear mask-dir-to-b ' +
        'mask-point-from-[80%] mask-to-0'}
    class:bg-black={isBlack} 
/>

// this would be simpler:
<header
    class={[
        'relative h-min landscape:lg:h-screen',
        'mask-linear mask-dir-to-b',
        'mask-point-from-[80%] mask-to-0',
       {'bg-black': isBlack} 
    ]}
/>

Tailwind is in my opinion, as somebody who has created custom design systems for years and tried out at least 6 different ways to organize complex styles, by far the most practical way to handle (complex) CSS. One of the very few downsides of Tailwind is the often large amount of classnames. The clsx-approach is a popular approach of a large and growing community of Tailwind-users.

brunnerh commented 1 year ago

One can add a linter when creating a SvelteKit project but it is entirely optional and not selected by default, so calling it the "default linter" is a bit misleading.

If a linter is just getting in the way by preventing valid and useful syntax, you are not doing yourself a favor by using it. I also did not suggest to just turn it off, but disable whatever rule is being triggered in this specific case.

I tried to find out what that might be but cannot reproduce any such issue with the code from my first comment. At least eslint, which is the option given by create-svelte, does not trigger.

So when comparing these two options:

<header
    class="relative h-min landscape:lg:h-screen
        mask-linear mask-dir-to-b
        mask-point-from-[80%] mask-to-0"
    class:bg-black={isBlack} />

<header
    class={[
        "relative h-min landscape:lg:h-screen",
        "mask-linear mask-dir-to-b",
        "mask-point-from-[80%] mask-to-0",
        { "bg-black": isBlack },
    ]} />

I do not think that the latter is better. It has much more punctuation and thus visual noise. (Also, instead of concatenating strings via + one could also use a template literal, which allows line breaks. But that should not be necessary when regular attribute syntax can be used instead.)

The only thing that is really missing, is support for multiple classes in class:..., but that is already being covered by #7170.

Also, you would still need something like clsx when dealing with class properties of components, so adding this special case for elements would be of limited value.

AndreasFaust commented 1 year ago

I tried out your example again without complains of Prettier/Eslint. I don’t know, why it was different the first time; maybe something else was wrong or I misinterpreted something. Anyway; combined with a solution for #7170, I think, the issue would be sufficiently resolved. Your last argument, that something like clsx would still be needed for components and the gain therefore very much limited, is completely right. I would respond: Maybe Vue has a point, by adding 'class' as a default attribute on every component, merging it with the class-names of the wrapping tag of the child-component. On the one hand, this would be handy, as I for example have to inject classes in most of the components, therefore repeating the same pattern all the time, on the other hand it leads to other problems, like having to wrap the "body" of a component in a single tag, which is thankfully not necessary in Svelte. Maybe there is another way I don’t know yet. In any case, thank you for responding so extensively and detailed and taking this topic so serious!