sveltejs / svelte

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

Inject multiple classes with class:x,y,z={condition} #3376

Closed marceloadsj closed 3 years ago

marceloadsj commented 4 years ago

Is your feature request related to a problem? Please describe. Basically, the needed to repeat the code when you want to inject multiple classes using the same condition, or extract the condition logic.

<div
  class='default-style'

  class:px-10={size === 'large'}
  class:py-5={size === 'large'}
>

// or

<script>
  // ...
  $: isLarge = size === 'large'
</script>

<div
  class='default-style'

  class:px-10={isLarge}
  class:py-5={isLarge}
>

Describe the solution you'd like I'm not sure if it's the best solution, but I was just thinking something like:

<div
  class='default-style'

  class:px-10,py-5={size === 'large'}
>

Describe alternatives you've considered For now, I'm using this approach to bypass:

<div class={`
  default-style
  ${size === 'large' ? 'px-10 py-5' : ''}
`}>

But it get big really quick, and if you mix different conditions, every time a single value changes, all the conditions need to be evaluated again.

How important is this feature to you? Well, I was just thinking to improve the flexibility and the possibilities of the class: syntax.

Additional context Maybe if we use something like a .split(',') and .map() on the toggle_class function, this feature can be added easily. Because, if I use class:x,y,z={true} today, svelte will inject the string x,y,z on the element, literally!

https://github.com/sveltejs/svelte/blob/8e62bd0b276c3d7e7bc6d61da96d82f187e3861e/src/runtime/internal/dom.ts#L233

Tks guys! 😄

Conduitry commented 4 years ago

That class:x,y,z={true} currently sets a class called x,y,z means that this would be a breaking change. A syntax like class:'a b c'={true} or class:"a b c"={true} is currently invalid and so would be eligible for this new feature. I don't personally feel particularly strongly in favor of adding this though, nor am I sure that the syntax with the quotes is a great idea.

marceloadsj commented 4 years ago

The css class x,y,z is invalid, so we don't have a breaking change on styles.

If you try to do something like:

<div class='x,y,z'>

<script>
const el = document.querySelector('.x,y,z')
</script>

el will be null, so we don't have breaking change on js selects!

Maybe the only scenario of a breaking change is a very specific thing like:

<div class='x,y'></div>

<script>
const el = document.querySelector('div')

const hasClass = el.className === 'x,y'
</script>

or something like that!

I think it's worth, considering the value and simplicify it can add to inject/remove bulk classes.

ghost commented 4 years ago

@marceloadsj The classname x,y,z is valid, you just forgot to escape the , characters in your selector:

<div class='x,y,z'>

<script>
const el = document.querySelector('.x\\,y\\,z')

console.log(el)
</script>

CSS classes can contain pretty much any character you like so long as it's not a space character, and with HTML5 the same applies to ids as well from what I remember.

marceloadsj commented 4 years ago

Hummm, makes sense. Tried here and confirmed!

On css it became:

.x\,y\,z {
  color: red;
}

Well, weird but, valid! hahaha

Maybe we can think in another solution of how this can be solved. Like the @Conduitry ideas.

Rich-Harris commented 4 years ago

In the past when faced with similar situations, we've added a compiler flag that lets you opt in to the sensible behaviour, but defaults to off, so that it's not a breaking change:

const compiled = svelte.compile(code, {
  commaSeparatedClassNames: true
});

Then in the next major version we drop the flag and disable the current behaviour.

I guess that could be a solution here? Don't know if it's too early to start thinking about v4.

I am curious about how often this need arises in real life — I don't think I've ever encountered a situation where I wanted to add two or more classnames based on a single condition.

marceloadsj commented 4 years ago

I think it fits really good with utility first css libs like: https://tailwindcss.com Actually, that's the main reason I started to think about that feature, haha!

About the flag, well, I think would be a really good solution! 👍

marceloadsj commented 4 years ago

Guys, I've added a msg on it. I will try to work in the next few days, sorry my delay: https://github.com/sveltejs/svelte/pull/3419

marceloadsj commented 4 years ago

WIP: https://github.com/sveltejs/svelte/pull/4349

antony commented 4 years ago

I think I'm in the camp of - this is extra complexity, when a more appropriate way to do this might be to use tailwind with postcss as documented on their site, to compose a set of classes into a single class, and then use that new class in your class: directive:

https://github.com/tailwindcss/designing-with-tailwindcss/blob/master/01-getting-up-and-running/05-composing-utilities-with-apply/css/tailwind.css

marceloadsj commented 4 years ago

For me, it's a good addition. Even because is an option (default disabled), so, don't have impact to who don't want to use it.

If you guys don't think it's worth, no problem for me! Then I can go as @antony suggested, using the proper tailwind (even having some downsides, like impact on purgecss..).

😄

george-oakling commented 3 years ago

Why not use it as array? class:[px-2, py-1]={codition}? It would be more understandable for me :)

antony commented 3 years ago

@george-oakling one of the goals of Svelte is to maintain full compatibility with regular HTML, regular CSS, and regular Javascript, which adding an array into the mix would prevent.

Conduitry commented 3 years ago

Sorry to let this sit for so long. I think that this technically being a breaking change, together with this being a somewhat confusing syntax, together with this encouraging use of Tailwind besides the 'proper' one of using the style preprocessor and purger - all together make this a 'no'. It's still easy to put an expression like {foo ? 'class1 class2' : 'class3 class4'} inside a class= attribute's value.

epavanello commented 3 years ago

I don't agree with this, I think that the ternary operator way is less readable and less intuitive.

<tr class={ !isLast ? 'border-b border-gray-200' : '' } >
// vs
<tr class:border-b,border-gray-200={!isLast} >

Please reconsider adding this feature, personally, I chose svelte due to the simplicity of doing this kind of things, compared with Angular, React or Vue.

paulovieira commented 3 years ago

For people using utility css classes (not necessarily tailwindcss), this feature would indeed be very welcome!

The need to change 2 (or more) classes depending a given condition happens frequently (which is natural since utility classes usually affect only 1 css property).

I was wondering if it would be feasible to implement this feature using babel (or even as rollup/webpack plugin?), which would "de-sugar" something like class:a,b,c to class:a class:b class:c. Anyone knows of some similar transformation to svelte code as a babel plugin?

Thanks.

epavanello commented 3 years ago

For people using utility css classes (not necessarily tailwindcss), this feature would indeed be very welcome!

The need to change 2 (or more) classes depending a given condition happens frequently (which is natural since utility classes usually affect only 1 css property).

I was wondering if it would be feasible to implement this feature using babel (or even as rollup/webpack plugin?), which would "de-sugar" something like class:a,b,c to class:a class:b class:c. Anyone knows of some similar transformation to svelte code as a babel plugin?

Thanks.

If you read the thread, someone has already implemented this optional feature but it was rejected. I hope that it will be re-accepted.

paulovieira commented 3 years ago

I had understood that it was rejected.

My comment to this closed issue had 2 objectives:

1) To give support to this feature, in case it is reconsidered in the future (I would guess that more and more people will need it, as the combination of svelte+tailwind grows in popularity)

2) To prospect the possibility of implementing the feature outside of svelte core. Is it feasible?

Thinking a bit more about 2), maybe can it can be done using svelte.preprocess and a html parser (the most popular/robust in nodejs world seem to be htmlparser2 and parse5). It's a area of svelte that I've never used, but I'm interested in exploring it.

Bastian commented 3 years ago

when a more appropriate way to do this might be to use tailwind with postcss as documented on their site

encouraging use of Tailwind besides the 'proper' one of using the style preprocessor

Adam Wathan (the creator of Tailwind) itself said, that the @apply feature only exists for users that are not familiar with the utlity-first workflow. He even advises against using it, so saying that it's the "appropriate way" is just not true. See https://twitter.com/adamwathan/status/1226511611592085504

It's very common to have multiple classes that depend on the same condition when using any utility-first CSS framework (not just Tailwind). I really hope that you reconsider your decision because otherwise you end up with code like this - or have to use the less-readable ternary operator: https://github.com/Bastian/bstats-web/blob/91923d8bbd0f73cfab299e203eea8e2214a2d22b/src/components/Step.svelte#L18-L25

abhijitgujar86 commented 3 years ago

In the past when faced with similar situations, we've added a compiler flag that lets you opt in to the sensible behaviour, but defaults to off, so that it's not a breaking change:

const compiled = svelte.compile(code, {
  commaSeparatedClassNames: true
});

Then in the next major version we drop the flag and disable the current behaviour.

I guess that could be a solution here? Don't know if it's too early to start thinking about v4.

I am curious about how often this need arises in real life — I don't think I've ever encountered a situation where I wanted to add two or more classnames based on a single condition.

I think this super important for the developers who use css frameworks like tailwind also i think many would prefer to reuse the css class rather than making new one all time.

Bastian commented 2 years ago

@Conduitry Sorry for the ping, but I assume that you don't get notified for the pervious comments as the issue is closed. It would be a shame if all the comments are unnoticed.

Judging by the comments after this issue has been closed (and the reactions), I would argue that there is enough interest in the feature to justify the additional complexity.

I agree that it's not worth a breaking change and prefer your proposed class:"a b c"={true} syntax. This is also what I intuitively tried first when running into this problem, before finding this GitHub issue. Since x,y,z is a valid CSS class, the class:x,y,z={true} syntax is imo a bad idea, not (only) because it would be a breaking change, but mainly because it would break compatability with some Tailwind classes (and probably other CSS frameworks as well). E.g., with Tailwind's JIT compiler you can have classes like grid-cols-[1fr,700px,2fr] which would not work with the class:x,y,z={true} syntax (unless we also add a way to escape the ,). The class:"a b c"={true} syntax appears to be much less error-prone with the additional benefit of not introducing a breaking change.

epavanello commented 2 years ago

What about this extended syntax? class:expression={boolean} Eg.

class:"a b c"={true}
class:{myClassesVariable}={myBooleanFlag}
class:{`my ${template} literal`}={false}
benwoodward commented 2 years ago

Just switched to Tailwind and ran into this issue, find myself missing a good solution for this.

What would make sense to me is:

class:{'button--highlight bold px-5 flash'}={condition}

I understand the goal of maintaining compatibility with HTML5, CSS + JS, however, this limitation potentially leads to:

class:one={condition1}
class:two={condition2}
class:three={condition1}
class:four={condition1}
class:five={condition1}
class:six={condition1}
class:seven={condition2}
class:eight={condition1}
class:nine={condition1}
class:ten={condition1}
class:eleven={condition1}
class:twelve={condition1}
class:thirteen={condition1}
class:fourteen={condition1}
class:fifteen={condition1}
class:sixteen={condition1}
class:seventeen={condition1}
class:eighteen={condition1}
class:nineteen={condition1}
class:twenty={condition1}
class:twentyone={condition1}
class:twentytwo={condition1}
class:twentythree={condition1}
class:twentyfour={condition1}
class:twentyfive={condition3}
class:twentysix={condition2}
michaeloliverx commented 2 years ago

This issue comes up all the time when using tailwind, I think it would be really useful.

nickpoulos commented 2 years ago

I don't think I've ever encountered a situation where I wanted to add two or more classnames based on a single condition

How is that even remotely possible? Tailwind is hardly the first utility based css framework.

  1. There are 15 devs saying this is important and incredibly useful, with clear examples of how the existing pattern leads to code like ^__.

  2. The suggested "solutions" are hacky and unreadable. String concats and ternarys all over? Babel preprocessors?

  3. There are several syntax solutions proposed that could get around the scary breaking changes.

Open this discussion back up!

frederikhors commented 2 years ago

Open this discussion back up!

I agree.

nostrorom commented 2 years ago

Just here to add my voice to the choir of Svelte+Tailwind users who'd find it very useful.

The current solutions, while functional, are not especially practical. @apply defeats the ability to fiddle with layout without having to leave your html line, and the ternary operator is a bit clumsy.

paulovieira commented 2 years ago

This feature can actually be accomplished with a svelte preprocessor. I created one here:

https://github.com/paulovieira/svelte-preprocess-class-directive

At the moment it lacks proper documentation and examples, but the essentials are working fine. I hope to clean the documentation this next weekend.

benwoodward commented 2 years ago

If this can be easily achieved via preprocessing, I'd argue it probably doesn't need to be a svelte feature. Even though I'm using Tailwind/Windi it still seems like a very rare use-case to me.

mastoj commented 2 years ago

I have been playing with svelte and sveltekit for roughly 30 minutes, and this was the first "issue" I wanted sorted since I do use tailwind. To lower the barrier of entry for people who use tailwind, which is increasing by the day, I think this should be addressed in a idiomatic way.

@paulovieira , when trying to do npm install of your package I get a 404.

RomanistHere commented 2 years ago

We can live without this feature, though I'd prefer to have it.

Can't say I like suggestion:

class:class-1,class-2={someVariable} - number of different symbols (-, _, ,) can easy mislead. Array is somehow better, but I understand why you don't want to use it.

Probably something like this would do it:

class:'class-1 class-2'={someVariable} - string 'class-1 class-2' is exactly what we would write in plain html. Zero misleading.

.class-1.class-2={someVariable} - is what we would write if we used Emmet to convert it to class - less clearer but somehow familiar

EDIT:

Added and example from "real life":

image

It would probably be cleaner with ifSomething ? 'class1 class2' : 'class3 class4' - but I don't really want to step out from class:something={isSomething} pattern.

silasabbott commented 2 years ago

I'm tacking on this problem I keep running into because I think it's highly related to this issue.

This works:

<div class:bg-red-500={boolean} />

These don't work:

<div class:bg-red-500/50={boolean} />
<div class:dark:bg-red-500={boolean} />

The current syntax won't allow me to add certain css classes dynamically because WindiCSS uses some characters that won't parse in this context. The solution for this issue could potentially fix this problem and provide support for other characters that utility-first tools introduce down the road.

RomanistHere commented 2 years ago

These don't work:

<div class:bg-red-500/50={boolean} />
<div class:dark:bg-red-500={boolean} />

This works for me: image Try to update dependencies. It's a Tailwind, but I don't think it matters since I tried to see if this works: class:something:new={true} - and it does, it renders something:new class on the element.

jvanderen1 commented 2 years ago

This issue is the main reason I opened this issue initially https://github.com/sveltejs/svelte/issues/7031 (although the devs don't seem to find it to be a popular solution).

Ideally, I would love for there to be as much reuse with specifying styles for a component.

Instead of a mess of classes declarations:

<script>
  export let disabled = false;
</script>

<button class='text-stone-900 text-lg font-serif' class:text-white="{!disabled}" class:bg-ocean-600="{!disabled}" >Submit button 1</button>
<button class='text-stone-900 text-lg font-serif' class:text-white="{!disabled}" class:bg-ocean-600="{!disabled}" >Submit button 2</button>
<button class='text-stone-900 text-lg font-serif' class:text-white="{!disabled}" class:bg-ocean-600="{!disabled}" >Submit button 3</button>

It would be nice to specify something close to emotion's syntax to achieve efficient class reuse:

<script>
  import styled from '@emotion/styled/svelte' // Not a real package

  export let disabled = false

  const SubmitButton = styled.button(() => [
    'text-stone-900 text-lg font-serif',
    !disabled && 'text-white bg-ocean-600'
  ])
</script>

<SubmitButton>Submit button 1</SubmitButton>
<SubmitButton>Submit button 2</SubmitButton>
<SubmitButton>Submit button 3</SubmitButton>

Or class injection driven by props to each individual component:

<script>
  import styled from '@emotion/styled/svelte' // Not a real package

  const SubmitButton = styled.button((props) => [
    'text-stone-900 text-lg font-serif',
    !props.disabled && 'text-white bg-ocean-600'
  ])
</script>

<SubmitButton disabled>Submit button 1</SubmitButton>
<SubmitButton>Submit button 2</SubmitButton>
<SubmitButton disabled>Submit button 3</SubmitButton>

(In Svelte's case, this hypothetical "styled" function could simply create a Svelte component derived from a base component and inject classnames into the class prop.)

below-1 commented 2 years ago

Yeahh... Either youre using utility css libs or not, multiple class binding is needed. Could you reopen this issue?

TristanBrotherton commented 1 year ago

There really needs to be a solution for this. What about just wrapping it in curly bracketsclass:{bg-red-700/50}={boolean}

bdavis-root commented 1 year ago

Another voice to chime in for support. If the name is valid in the class attribute, there needs to be a way of using the class: directive for that name. IE: class:{bg-[#163a58]/[.8]}={isLightMode}

NonVideri commented 8 months ago

Yes, this would be a really useful feature, in particular for using Svelte with things like TailwindCSS.

antony commented 8 months ago

We closed this bug a while ago with our decision and reasoning. There is no value in adding support to closed requests.