Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork4.6k
feat: allow objects/arrays for class attribute#14714
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
changeset-botbot commentedDec 15, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
🦋 Changeset detectedLatest commit:7650bb4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means?Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
preview:https://svelte-dev-git-preview-svelte-14714-svelte.vercel.app/ this is an automated message |
Uh oh!
There was an error while loading.Please reload this page.
ottomated commentedDec 18, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I really love this feature. I'm not super knowledgeable in css performance, but would it be worth it eventually to precompile optimized versions of this? For example, if svelte sees a static object key at compile time: <divclass={{'a b c':condition,d:condition2 }}></div> it could generate something like this: $.toggle_classes(div_1,["a","b","c"],condition);$.toggle_class(div_1,"d",condition2); which is more fine grained and avoids setting the entire className when one condition changes. Any more complicated dynamic classes could still use clsx under the hood: <divclass={{foo:condition, [dynamic_classes]:condition2 }}></div> $.toggle_class(div_1,"foo",condition);$.set_class(div_1,$.clsx({[dynamic_classes]:condition2}),""); |
Alternatively, would it simplify things to compile directly to a ternary statement? <divclass={{foo:condition, [dynamic_classes]:condition2 }}></div> could become $.set_class(div_1,(condition ?"foo" :"")+(condition2 ?`${dynamic_classes}` :""),""); which seems like it would be more performant and remove the need for some of the metadata that this feature added. |
I would need to perf test this to be sure, but I'm pretty certain that the runtime overhead of this is negligible. Turning specific statements into ternaries is interesting, but again I'm unsure if the perf impact is visible, also it results in a bit more code per component |
True, it does add a bit of code size, although it does enable rollup to treeshake it (the pattern of |
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Conduitry <git@chor.date>
Hi, Just a little question : $.set_class(div,$.clsx(clazz),"") Any reason to call |
If the compiler is certain that it's unnecessary, then it doesn't get added. Putting it inside set_class would mean it was included in the bundle for every app that has a |
wbudd commentedDec 24, 2024
Great to have this feature! Question: the doc text of this PR states:
But Svelte 5.15 was released a few days ago it and doesn't contain this yet it seems... deferred to 5.16 maybe? (Just eager to get my hands on this one!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks, fixed (you won't have to wait much longer)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
015210a
intomainUh oh!
There was an error while loading.Please reload this page.
orefalo commentedDec 26, 2024
Great, can you make it work with custom components?
|
It does. |
TehBrian commentedFeb 26, 2025
Out of curiosity, are there any plans to deprecate/remove the |
I believe the intention is to eventually deprecate it and then later remove it, but I don't know that there's a specific timeline in mind for that. |
IMHO I think that the style directive is still useful, since it actually takes precedence over the class attribute, and allow to remove classnames, which is not possible with clsx() alone. For example : <script>let active=$state(false);</script><divclass={["active", {active}]}></div><divclass="active"class:active></div> Will output : <divclass="active"></div><div></div> So |
TehBrian commentedFeb 27, 2025
That's an interesting example, but is there a use-case for wanting to remove classes from the class list (instead of just controlling them directly)? |
It could be usefull if you spread the class, but want to have the control on some class name. |
Uh oh!
There was an error while loading.Please reload this page.
This basically implementshttps://github.com/lukeed/clsx within Svelte, allowing people to use objects/arrays to conditionally set classes. This has a couple advantages over
class:
, including:class:list
directive #12610)Before submitting the PR, please make sure you do the following
feat:
,fix:
,chore:
, ordocs:
.packages/svelte/src
, add a changeset (npx changeset
).Tests and linting
pnpm test
and lint the project withpnpm lint