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

Feat: Add support for class list #11299

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RaiVaibhav
Copy link
Contributor

@RaiVaibhav RaiVaibhav commented Apr 23, 2024

Svelte 5 rewrite

Fixes: #7294
Proposing to support classList feature inside the class of svelte.

After digging solid.js I realized there are two ways to support this

  • Non svelte way? : support a new classlist attribute in the element and then remove it after these classlist is set in the DOMTokenList
<script>
	let focused = $state(false);

</script>

<button classlist={{abc: focused}}>
	click
</button>
  • With the insight from the @dummdidumm I realized there can be another way to support without interfering the current syntax, since value can either be Expression tag or Text, means the value can be ObjectExpression, and since we already had set_class for csr and attr for ssr, I just to modify both where csr support DOMTokenList toggle and for ssr it can simply be evaluated string, that means, there is no change in the current structure
<script>
	let focused = $state(false);

</script>

<button class={"abc"}>
	click
</button>

But now it will also support the object syntax

<script>
	let focused = $state(true);
</script>

<button class={{'abc': focused }}
	onclick={() => focused = !focused}
>
	abc
</button>

But again, is this valid syntax ?, it feels like mixing jsx here and I feel few bottles neck is the hydration process.

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 with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Copy link

changeset-bot bot commented Apr 23, 2024

⚠️ No Changeset found

Latest commit: 1d81ca4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@RaiVaibhav RaiVaibhav force-pushed the feat/add-support-for-class-list branch from eda555e to 1d81ca4 Compare April 23, 2024 15:40
@Antonio-Bennett
Copy link

I’ll be the first to tell you that syntax is rubbing me the wrong way.

@RaiVaibhav
Copy link
Contributor Author

I can relate @Antonio-Bennett generally class inside the HTML has been see as a string only, even I felt like itchy before opening up the proposal, but it's just a proposal, it's just for opening up the discussion or some start, it can be closed or taken forward to by the maintainers and contributors.

@RaiVaibhav
Copy link
Contributor Author

Just forgive me in advance

@RaiVaibhav
Copy link
Contributor Author

Continuing the yesterday, I figured if new classlist attribute to be used then for

csr: approach will be to toggle the DOMTokenlist and remove the classlist attribute, after updating the token

ssr: the classlist attribute will be converted from Object Expression to TemplateLiteral i.e., for the above component the parsed component will look something like? (Same as what solid js does?)

export default function App($$payload, $$props) {
	$.push(true);

	let focused = true;

	$$payload.out += `<button${$.attr("class", `${focused === true ? 'abc' : ""} `, false)}>abc</button>`;
	$.pop();
}

And this was kind of achievable after transforming the AST from Object expression to Template literal

			var template = {
				type: 'TemplateLiteral',
				quasis: [],
				expressions: []
			}
			if (name === 'classlist') {
				const abc = attribute.value[0].expression;
				const properties =  abc.properties
				for (var i = 0;i<properties.length;i++) {
					var {key: left, value: right } = properties[i];
					var c = b.conditional(right, left, b.literal(''))
					template.expressions.push(c);
					template.quasis.push(b.quasi(' '))
				}
			}
			context.state.template.push(
				t_expression(
					b.call(
						'$.attr',
						b.literal(name),
						template,
						b.literal(is_boolean)
					)
				)
			);

@MotionlessTrain
Copy link

I'm wondering how breaking this is: Will everyone need to update everywhere where classes are used to now use object expressions? And in case of tailwind (where it is not uncommon to have 10+ classes on an element), wouldn't this get too bulky if this needs to be changed to {'w-5': true, 'b-0': true, /* ?.. */}?

@RaiVaibhav
Copy link
Contributor Author

No, it doesn't break the existing structure, and why would it be, it just check if the class value is object expression or not and according iterate over key value

@david-plugge
Copy link

Not sure if this is really needed, you can use class:whatever={true} already and for more advanced cases you could create a little helper or make use of a library that does what you describe.

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

Successfully merging this pull request may close these issues.

Dynamic conditional CSS classes
4 participants