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

breaking: use <svelte-css-wrapper> instead of <div> for style props #13499

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Oct 4, 2024

Fixes #13490. This is a breaking change, so I've also documented the change too.

Copy link

changeset-bot bot commented Oct 4, 2024

🦋 Changeset detected

Latest commit: b318f87

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

This PR includes changesets to release 1 package
Name Type
svelte Patch

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

@fcrozatier
Copy link
Contributor

What about <H1 --color="blue" /> or <Article --color="blue" /> where we will have heading or sectionning content inside a span (which has phrasing content model)?

@trueadm
Copy link
Contributor Author

trueadm commented Oct 4, 2024

Yeah, we're going to have to use another element it seems.

@trueadm trueadm marked this pull request as draft October 4, 2024 13:11
@dummdidumm
Copy link
Member

sounds like we need to use a bogus custom element like svelte-css-wrapper or sth, which hopefully can appear anywhere?

@trueadm
Copy link
Contributor Author

trueadm commented Oct 4, 2024

Actually hold on, it seems that <span> can wrap <h1> just fine. It's just that I had a <p> element around it all that was causing the issue. Interestingly though, the <span> does cause the invalidation to not work right, whilst using a custom element does. So I'll switch to that.

@trueadm trueadm marked this pull request as ready for review October 4, 2024 13:21
@trueadm trueadm changed the title breaking: use <span> instead of <div> for style props breaking: use <svelte-css-wrapper> instead of <div> for style props Oct 4, 2024
@ottomated
Copy link
Contributor

The name feels a bit clunky, is there something shorter that could work?

@trueadm
Copy link
Contributor Author

trueadm commented Oct 5, 2024

it could be a random name like <svelte-abc> where abc is a 3 letter random lowercase character string.

@dummdidumm
Copy link
Member

The more characters the less chance of accidentally using an actual custom element. I'm not sure if aesthetics really matter here.

@Conduitry
Copy link
Member

The reason that this is a breaking change is that someone could (misguidedly) be using the existing <div> in some CSS selector. If we make the element name be randomized, that would make that sort of thing a lot harder to do, and I can't decide whether that means it would be a good idea or a bad idea to do.

@ottomated
Copy link
Contributor

Could it be called <svelte-hash> where the hash is the same that would be used for css classes in the component?

@trueadm
Copy link
Contributor Author

trueadm commented Oct 5, 2024

Could it be called <svelte-hash> where the hash is the same that would be used for css classes in the component?

No that contains invalid element tag characters

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.

node_invalid_placement_ssr caused by --styled-props
5 participants