feat: better dynamic component css props#11792
feat: better dynamic component css props#11792Rich-Harris merged 5 commits intosveltejs:mainfrom paoloricciuti:better-dynamic-component-cssprops
Conversation
🦋 Changeset detectedLatest commit: 01e8afb 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 |
|
I'm not sure what kind of test is needed here....this is about the output but i remember we don't want to overdo tests on the compiler output right? |
| component.componentName = 'Slider2'; | ||
| assert_slider_2(); | ||
| component.componentName = undefined; | ||
| assert.equal(window.document.querySelector('div'), null); |
There was a problem hiding this comment.
Was confused by this deletion, and checked the current Svelte 4 REPL once more:
- when
thisis falsy initially, there's adiv - but if it's switching truhy from to falsy, then it will remove the wrapping
div
--> feels like a bug in Svelte 4
--> what does this say about this change? Can we do this change after all, or not? I think having it falsy initially is very rare, so very few people would've run into this bug before. Always having the div might catch people's CSS selectors offguard.
There was a problem hiding this comment.
I actually changed this to check that the div doesn't have a firstElementChild. But your reasoning still applies. That said i think the situations where this can trip people are the same where the component is actually there (more or less) which might be a bit more now that :has it's a bit more standard but shouldn't be that much because it's just if you are writing css based on the fact that a certain element follow another.
There was a problem hiding this comment.
Could it be worth passing the component to css_props to allow css_props to check for is thrutyness? And by that i mean literally the component that would be rendered...so for a normal component it would just be the component itself, for SvelteComponent the argument of this
There was a problem hiding this comment.
I'm inclined to ignore the falsy <svelte:component> edge case. As noted, it's already buggy (and to my knowledge no-one has encountered this bug in the wild), and even if it wasn't, the cases where your styles are affected by the presence of an unexpected <div style="display: contents"> with no children are vanishingly rare and easily worked around.
In other words I think it's fine to always have the wrapping div
|
thanks! |
Svelte 5 rewrite
Closes #11788
Marking momentarily as draft because i need toPlease note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (
main).If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is
svelte-4and notmain.Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.Tests and linting
pnpm testand lint the project withpnpm lint