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

Feature request: class directives #890

Closed
jacwright opened this issue Oct 10, 2017 · 25 comments
Closed

Feature request: class directives #890

jacwright opened this issue Oct 10, 2017 · 25 comments

Comments

@jacwright
Copy link
Contributor

jacwright commented Oct 10, 2017

Proposal for a directive to toggle classes.

<div class:active="thing.isActive()" class:selected="selectedItem === thing">
  ...
</div>

Adding and removing a class based off of boolean state is a very common pattern. This directive would only be a nice-to-have since there are several alternatives currently in use.

Alternatives:

  • the class attribute, e.g. <div class="thing {{getClasses()}}"> with a bunch of class logic in a component method—my preference is to put the logic in the template
  • data attributes, e.g. <div data-active="{{thing.isActive()}}">—cons, many existing libraries (ahem bootstrap) already use classes, and the css is marginally nicer with classes (.active vs [data-active="true"])

The existing solutions are not bad, so this is very much a nice-to-have. But the little things do make a difference. Thoughts?

@jacwright jacwright changed the title Feature request, class directives Feature request: class directives Oct 10, 2017
@Rich-Harris
Copy link
Member

I'm ambivalent about this. Normally I use ternary expressions in these situations. On the one hand a class: directive does result in slightly leaner code, which I'm a fan of:

<div class:active="thing.isActive()"></div>
<div class="{{thing.isActive() ? 'active' : ''}}"></div>

On the other, it's another extra piece of syntax to learn, which is potentially off-putting — the ternary expression is immediately clear to anyone who knows JS.

I'd be interested to know what everyone else reckons. Yay or nay?

@tivac
Copy link
Contributor

tivac commented Nov 19, 2017

We've been doing these as computed properties to keep the view code easier to read.

@coussej
Copy link

coussej commented Dec 20, 2017

I'd be interested to know what everyone else reckons. Yay or nay?

YAY!

@jacwright
Copy link
Contributor Author

I'm also a YAY for a class directive. It is one more thing, but an optional one, and one which would be used in every SPA I've ever worked on. Toggling classes is very common, and I still prefer it over attribute selectors in my CSS.

@eyeseast
Copy link

I've been doing computed properties and terneries, and it hasn't been a problem. The only thing that's been weird is having this sort of thing:

{{#each things as t, i }}
<p class="{{ i == 2 ? 'active' : 'inactive' }}">{{ thing }}</p>
{{/each}}

I end up with either this unused class name (inactive in this case) or class="" on a bunch of elements. Neither is really a problem, but both are sort of weird.

@jacwright
Copy link
Contributor Author

There are work arounds, for sure. And they're not awful. But <div class:active="thing.isActive()"></div> is just clean and readable and a great improvement for a common use-case.

@tivac
Copy link
Contributor

tivac commented Dec 20, 2017

Could this format be tweaked to allow multiple classes per check? Or to allow dynamic values for the class? Right now it seems really limited.

@jacwright
Copy link
Contributor Author

jacwright commented Dec 20, 2017

It is limited to the most common use-case. If you have suggestions that allow more flexibility without making the syntax more difficult to read, I would be interested to hear your thoughts. We don't want to provide a syntax that is no cleaner than what we have now, just more to learn without the benefit.

@jacwright
Copy link
Contributor Author

If the directive uses elem.classList.toggle('active', expr()) then you could have both.
<div class="list-item" class:selected="selected"></div>

@jacwright
Copy link
Contributor Author

jacwright commented Feb 23, 2018

Please vote on this comment whether you'd like to see a class directive as described above added to Svelte using 👍 and 👎 .

@arxpoetica
Copy link
Member

I say nay to directives, yay to computed properties. Attitude about it: feature/scope creep. Less is more.

@silentworks
Copy link

I think this is the beginning of something we won't like in the future. You can already do what you want this syntax to do in current codebase, why introduce this shortcut with this syntax which is only going to confuse folks?

<MyComponent class:active="this.isActive()" :foo :bar hello="World" class:selected="this.isSelected()" />

To me the above would leave me in such confusion and if class: can't work on components I would then start to wonder why is this thing so inconsistent. One of the reasons why the above wouldn't work is that Svelte components does not require 1 root level item, so how would Svelte work out where to apply this class?

@fiskgrodan
Copy link
Contributor

What about something similar to Vue object syntax?: https://vuejs.org/v2/guide/class-and-style.html#Object-Syntax

Object syntax:
<div :class="{ active: thing.isActive(), error: thing.hasError() }"></div>

@jacwright
Copy link
Contributor Author

jacwright commented Feb 25, 2018

@silentworks I think the transition directive only works on components as well. I hope this isn't the beginning of something we don't like. I would like to just see this one directive added for a very common use-case, and assuming it is documented, it shouldn't confused anyone any more than any other binding.

@fiskgrodan I think that is a decent syntax as well, though it collides with the 1-way binding syntax.

@opus131
Copy link

opus131 commented Feb 25, 2018

If it's only to set a class or not, I'd usually use {{thing.isActive() && 'active'}} rather than a ternary.

IMHO the difference in terms of typed chars is in that case not big enough to warrant new syntactic sugar:
<div class:active="thing.isActive()"></div>
<div class="{{thing.isActive() && 'active'}}"></div>

Plus, if you later realise that you need more than just an on/off toggle, you'll have more to refactor if you used the proposed new directive. So I'll vote nay.

@jacwright
Copy link
Contributor Author

Looks like the nays have it. Thank you for all your input!

@jacwright
Copy link
Contributor Author

You know, the vote was pretty even. It wasn't a landslide in either direction. And other frameworks have support like this such as ractive's class-*. What if it were supported but not documented? 😄

Isn't one of the benefits of Svelte that your app won't bloat with features you don't use, but those who do want to use them can? Would a class:active="isSelected" directive be something that could be added as a 3rd party plugin? (Svelte doesn't handle 3rd-party plugins for the compilation phase yet, does it?)

As I've been doing real app work with Svelte (refactoring an existing app), I'm really missing the cleanliness of my template code with this feature, and I would love it if it were something I could have somehow without negatively affecting those who do not want it.

@arxpoetica
Copy link
Member

I actually changed my vote to yay after thinking long and hard about it. Your point about it being additive—people can still do ternaries!—without it breaking anything. I suspect I would actually end up using this a lot.

However, I do think it should be documented if we do it, even if it's in some remote corner of doculand.

@arxpoetica arxpoetica reopened this Mar 14, 2018
@jacwright
Copy link
Contributor Author

A problem with {{thing.isActive() && 'active'}} is you get class names like false or undefined depending on the output of isActive. Svelte doesn't try and turn falsey bindings into empty strings so we have to use ternaries or add || '' at the end which is about the same.

The real benefit is when you have 2+ dynamic classes on an element. Having many ternaries gets messy.

Computed doesn't help for elements inside #each blocks. And creating new components to support this case can explode the number of components you have making your application more complex to find your way around.

jacwright added a commit that referenced this issue Aug 24, 2018
Allows `<div class:active="user.active">` to simplify templates littered with ternary statements.

Addresses #890
@jacwright
Copy link
Contributor Author

I've implemented this feature in #1685. It uses classList.toggle and should have minimal size impact in apps that use it and no impact in apps that don't.

Rich-Harris added a commit that referenced this issue Aug 25, 2018
@Rich-Harris
Copy link
Member

Thanks @jacwright! Released in 2.13

kaisermann pushed a commit to kaisermann/svelte that referenced this issue Sep 19, 2018
@garygreen
Copy link

garygreen commented Jun 1, 2019

The real benefit is when you have 2+ dynamic classes on an element. Having many ternaries gets messy.

This is something we're discovering whilst checking out possibility of migrating away from Vue app to Svelte.

We use a utility-first css approach which means we have many classes applied to an element. In vue this was convenient because we could just do:

<div
    class="block py-4 px-6 hover:bg-blue hover:text-white cursor"
    :class="{ 'active bg-blue text-white': i == activeIndex }"
>

However in Svelte we need to add each utility separately which is a bit clunky:

<div
    class="block py-4 px-6 hover:bg-blue hover:text-white cursor"
    class={ i == activeIndex ? 'active' : '' }
    class={ i == activeIndex ? 'bg-blue' : '' }
    class={ i == activeIndex ? 'text-white' : '' }
>

We're possibly missing some other trick to simplify things though, like a computed property or something. But ultimately if your coming from Vue background the workflow does feel a bit fiddly in this regard.

Also if your adding classes through the class.active= style shortcut I'm sceptical what kind of special characters it will allow - in the case of Tailwind it uses : in classnames, so would it allow something like this? Technically in CSS you can have any characters as long as you escape them, I believe.

class.hover:bg-blue={ active }

@kylecordes
Copy link

@garygreen Take a look at the pull request merged above; after the comment earlier about "the nays have it", nonetheless a class directive has been added, and is very nice. (I don't know whether it handles class names that include the full breadth of characters allowed by CSS.)

https://svelte.dev/tutorial/classes

@mrkishi
Copy link
Member

mrkishi commented Jun 2, 2019

@garygreen In addition, if you have suggestions on how to improve the class directive without conflicting with the current use cases I'm sure everyone would love to hear them, so feel free to open an issue on that.

For now, if you have a single conditional responsible for multiple classes, you could use a reactive declaration to update your class string once instead of using multiple class directives. Check out an example.

Regarding special characters: they work, but with the class: directive there's currently a bug with scoped styles. I've opened #2929 for this, but you can still reference outside styles such as those from Tailwind even while that's not fixed.

@narenderv7
Copy link

narenderv7 commented Apr 11, 2020

The real benefit is when you have 2+ dynamic classes on an element. Having many ternaries gets messy.

This is something we're discovering whilst checking out possibility of migrating away from Vue app to Svelte.

We use a utility-first css approach which means we have many classes applied to an element. In vue this was convenient because we could just do:

<div
    class="block py-4 px-6 hover:bg-blue hover:text-white cursor"
    :class="{ 'active bg-blue text-white': i == activeIndex }"
>

However in Svelte we need to add each utility separately which is a bit clunky:

<div
    class="block py-4 px-6 hover:bg-blue hover:text-white cursor"
    class={ i == activeIndex ? 'active' : '' }
    class={ i == activeIndex ? 'bg-blue' : '' }
    class={ i == activeIndex ? 'text-white' : '' }
>

We're possibly missing some other trick to simplify things though, like a computed property or something. But ultimately if your coming from Vue background the workflow does feel a bit fiddly in this regard.

Also if your adding classes through the class.active= style shortcut I'm sceptical what kind of special characters it will allow - in the case of Tailwind it uses : in classnames, so would it allow something like this? Technically in CSS you can have any characters as long as you escape them, I believe.

class.hover:bg-blue={ active }

When we add multiple classes in elements, as you did, svelte complaining duplicates attributes.

you can use something like this,

 <div class="class="block py-4 px-6 hover:bg-blue hover:text-white cursor { i == activeIndex ? 'active bg-blue text-white' : '' }"></div >

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests