Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
Rich-Harris merged 21 commits intomainfromclass-enhancements
Dec 24, 2024

Conversation

dummdidumm
Copy link
Member

@dummdidummdummdidumm commentedDec 15, 2024
edited
Loading

This basically implementshttps://github.com/lukeed/clsx within Svelte, allowing people to use objects/arrays to conditionally set classes. This has a couple advantages overclass:, including:

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC:https://github.com/sveltejs/rfcs
  • Prefix your PR title withfeat:,fix:,chore:, ordocs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.
  • If this PR changes code withinpackages/svelte/src, add a changeset (npx changeset).

Tests and linting

  • Run the tests withpnpm test and lint the project withpnpm lint

janosh reacted with thumbs up emojiqwuide, JetLua, Serator, jjones315, Devr-pro, harryqt, wbudd, diegoulloatrio, GauBen, neoncitylights, and 18 more reacted with hooray emojihenrikvilhelmberglund, stephane-klein, jchild3rs, and matschik reacted with heart emojidiegoulloatrio, rossrobino, neoncitylights, phenomen, flayks, Blatts12, and vnphanquang reacted with rocket emojimax-got, xiBread, qwuide, david-plugge, hanszoons, Ocean-OS, xeho91, neoncitylights, and EdricChan03 reacted with eyes emoji
@changeset-botchangeset-bot
Copy link

changeset-botbot commentedDec 15, 2024
edited
Loading

🦋 Changeset detected

Latest commit:7650bb4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
svelteMinor

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

@github-actionsGitHub Actions
Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@14714

@Rich-Harris
Copy link
Member

preview:https://svelte-dev-git-preview-svelte-14714-svelte.vercel.app/

this is an automated message

@dummdidummdummdidumm changed the titleclass stufffeat: allow objects/arrays for class attributeDec 17, 2024
@ottomated
Copy link
Contributor

ottomated commentedDec 18, 2024
edited
Loading

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}),"");

@ottomated
Copy link
Contributor

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.

@dummdidumm
Copy link
MemberAuthor

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

harryqt reacted with thumbs up emoji

@ottomated
Copy link
Contributor

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 ofclsx({ 'foo': true' }) is common).

@dummdidummdummdidumm mentioned this pull requestDec 18, 2024
6 tasks
@Rich-Harris
Copy link
Member

going to tweak the docs because this looks a bit nutty 😆

image
huntabyte, harryqt, jarrodldavis, and elron reacted with laugh emoji

Co-authored-by: Conduitry <git@chor.date>
@adiguba
Copy link
Contributor

Hi,

Just a little question :<div class={clazz}> will generate something like this :

$.set_class(div,$.clsx(clazz),"")

Any reason to call$.clsx() here ?
Couldn't this be done insideset_class() ?

@Rich-Harris
Copy link
Member

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 aclass attribute

adiguba and RyanYANG52 reacted with thumbs up emoji

@wbudd
Copy link

Great to have this feature!

Question: the doc text of this PR states:

Since Svelte 5.15,class can be an object or array, and is converted to a string usingclsx.

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!)

Copy link
MemberAuthor

@dummdidummdummdidumm left a 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)

wbudd reacted with thumbs up emoji
@Rich-HarrisRich-Harris marked this pull request as ready for reviewDecember 24, 2024 13:58
@Rich-HarrisRich-Harris merged commit015210a intomainDec 24, 2024
11 checks passed
@Rich-HarrisRich-Harris deleted the class-enhancements branchDecember 24, 2024 13:58
@github-actionsgithub-actionsbot mentioned this pull requestDec 24, 2024
@orefalo
Copy link

Great, can you make it work with custom components?

<Navbar    class={{        'fixed start-0 top-0': true,        'border-b': scrollPos,        'bg-gray-50': !scrollPos    }}>

@Rich-Harris
Copy link
Member

It does.NavBar just needs to useprops.class

orefalo, BCsabaEngine, kamranasad7, and jacksteamdev reacted with thumbs up emoji

@TehBrian
Copy link

Out of curiosity, are there any plans to deprecate/remove theclass: directive, even if just for the sake of having one good, idiomatic way of doing things?

@Conduitry
Copy link
Member

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.

@adiguba
Copy link
Contributor

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>

Soclass:directives allows the component to keep control of some classes.

Exemple in REPL

navtoj reacted with thumbs up emoji

@TehBrian
Copy link

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)?

matschik reacted with thumbs up emoji

@adiguba
Copy link
Contributor

It could be usefull if you spread the class, but want to have the control on some class name.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@Rich-HarrisRich-HarrisRich-Harris approved these changes

@ConduitryConduitryConduitry left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
8 participants
@dummdidumm@Rich-Harris@ottomated@adiguba@wbudd@orefalo@TehBrian@Conduitry

[8]ページ先頭

©2009-2025 Movatter.jp