-
-
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
feat: allow objects/arrays for class attribute #14714
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 39687b4 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 |
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: <div class={{ '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: <div class={{ 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? <div class={{ 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 |
Co-authored-by: Conduitry <[email protected]>
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 |
This basically implements https://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