-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Comments
I'm ambivalent about this. Normally I use ternary expressions in these situations. On the one hand a <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? |
We've been doing these as computed properties to keep the view code easier to read. |
YAY! |
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. |
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 ( |
There are work arounds, for sure. And they're not awful. But |
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. |
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. |
If the directive uses |
Please vote on this comment whether you'd like to see a class directive as described above added to Svelte using 👍 and 👎 . |
I say nay to directives, yay to computed properties. Attitude about it: feature/scope creep. Less is more. |
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?
To me the above would leave me in such confusion and if |
What about something similar to Vue object syntax?: https://vuejs.org/v2/guide/class-and-style.html#Object-Syntax Object syntax: |
@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. |
If it's only to set a class or not, I'd usually use IMHO the difference in terms of typed chars is in that case not big enough to warrant new syntactic sugar: 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. |
Looks like the nays have it. Thank you for all your input! |
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 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. |
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. |
A problem with 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. |
Allows `<div class:active="user.active">` to simplify templates littered with ternary statements. Addresses #890
I've implemented this feature in #1685. It uses |
Thanks @jacwright! Released in 2.13 |
Adds the class directive
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
|
@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.) |
@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 |
When we add multiple classes in elements, as you did, svelte complaining duplicates attributes. you can use something like this,
|
…ple (sveltejs#890) fix state_proxy_equality_mismatch error
Proposal for a directive to toggle classes.
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:
<div class="thing {{getClasses()}}">
with a bunch of class logic in a component method—my preference is to put the logic in the template<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?
The text was updated successfully, but these errors were encountered: