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

Implement RFC 13 #5628

Closed
Rich-Harris opened this issue Oct 31, 2020 · 11 comments · Fixed by #6237
Closed

Implement RFC 13 #5628

Rich-Harris opened this issue Oct 31, 2020 · 11 comments · Fixed by #6237

Comments

@Rich-Harris
Copy link
Member

sveltejs/rfcs#13

@zamanruhy
Copy link

Honestly, looks bad. It will wrap my component markup with extra div which will break selectors like a > b. I will just not use this feature.

@kevmodrome
Copy link
Contributor

kevmodrome commented Oct 31, 2020

@zamanruhy The RFC addresses this, essentially it comes down to using display: contents. Specifically this bit explains it:

display: contents essentially removes the wrapper element from the DOM, but allows it to set inheritable styles including custom properties. It's easier to show than tell. It's supported in all modern browsers (ignore notes 2 and 3, they don't apply in this situation), including Edge when the Chromium version ships.

Edit: I see now that this is not at all what you were commenting on. The times you would run into this is when you would do .parentClass > :global(.childClass). Or if you had a global stylesheet that had a selector like that. A workaround to this is to use a b to select things.

@zamanruhy
Copy link

zamanruhy commented Oct 31, 2020

@kevmodrome Yes, that's exactly what I meant. Regardless of any workaround, I don't like the idea of adding an extra node without my knowledge. The good thing is this is an optional feature and doesn't add any code to runtime ...

@idoros
Copy link

idoros commented Nov 1, 2020

Notice that this will make targeting a component that doesn't want to be targeted even easier:

<style>
    [style*=--starRating] > * {}
</style>
<StarRating --starRating />

Edit: The RFC mentions under drawbacks the idea of allowing a component to explicitly expose its "style properties" for validation and completions. that would help reduce marking an instance like this, and potentially you could then namespace the properties (although with namespace you would lose global theming - that can probably be handled differently anyhow in order to achieve validation and completions as well).

@TheComputerM
Copy link

This feature would definitely be useful but something feels odd/bad about this approach.

@jonatansberg
Copy link

jonatansberg commented Nov 1, 2020

I know the RFC already been merged, and I don't mean to drag this on. I do really like the idea of embracing CSS variables for this, but at the same time I share the concern about the use of display: contents and adding an extra wrapper element.

Wouldn't treating --prop={value} as syntactic sugar for style={`--prop: ${value};`} and limit the usage to actual elements hit a nice middle ground? To target a (third party) component you would then have to introduce a wrapping element yourself. This mirrors how classes already work and avoids the issues with introducing an unexpected DOM node.

Implementing the above would also make it possible to achieve behavior described in the RFC as part of a theming or design system library like this:

<script>
  let vars = {}
  $: spread = Object.fromEntries(
    Object.entries(vars).map([key, val] => ['--' + key, val])
  )
</script>
<style>
  theme-context { display: contents; }
</style>
<theme-context {...spread}>
  <slot />
</theme-context>

This has the advantage of making the use of an extra element a bit more explicit.

Support for targeting components directly could be also achieved by exposing passed css vars as a special prop (eg $$vars) that then can be spread on to the appropriate element inside the component. Although, really any way to spread an object as css vars would solve that.

Edit: Something like the Theme component above could also be added to Svelte as say <svelte:theme vars={...}> or similar if you want to even further embrace this as a part of Svelte itself.

@PaulMaly
Copy link
Contributor

PaulMaly commented Nov 1, 2020

I believe that keeping the status quo is better than implementing this RFC. But if it's inevitable, I prefer the first variant of the implementation described here. The previous variant looks good in comparison with the current one. At least it doesn't imply implicit creation of additional elements and a huge number of side effects.

@raythurnvoid
Copy link

raythurnvoid commented Nov 2, 2020

I too have some serious doubts about syntax of this RFC, write --bg-color="#000" on a component doesn't feel right and i don't understand what benefits it has against using a style prop (eg: style="--bg-color=#000"), in fact I would have been much more inclined to go for "Do nothing, but encourage the use of custom properties" way.

Moreover this extra wrapper div seems very bad, as others appointed, it breaks css selectors.
I think that if someone want a wrapper div around it's component, this should be accomplished through declaring clearly with a svelte:option like configuration similarly to how custom components are handled. For example this configuration could create a wrapper div even if customElement: false:

<svelte:options tag="my-comp" />

This would allow someone else to write something like this (without :global):

<style>
  my-comp {
    background-color: #000;
  }
</style>

<MyComp />

Of course the compiler should do some work to understand that my-comp and MyComp refer to the same thing.

Getting back to what stated in the RFC, another nice feature is to have props working in style tag, this would be great but i understand the difficulties on implement such a thing.

@TheComputerM
Copy link

I feel something like this would be better

<script>
export let color = 'red';
</script>

<style>
div {color: var(color);}
span {color: var(--color)}
</style>

<div>
  <span />
</div>

OUTPUT

<div class="svelte-nerdboi" style="--color: red;">
  <span class="svelte-nerdboi" />
</div>

<style>
div {color: var(--color);}
span {color: var(--color);}
</style>

@Rich-Harris
Copy link
Member Author

I'm going to lock this conversation as it's unproductive. The RFC repo (where the reaction was strongly positive) is the place for discussion about what features to add; the decision has been made.

I will quickly address some of the feedback here though:

It will wrap my component markup with extra div which will break selectors like a > b

This is untrue. Since this syntax applies to components, not elements, we're talking about selectors that cross a component boundary. The only way CSS selectors are affected is if you have one like this a > :global(b), which frankly is a bad selector anyway, since the parent is making assumptions about the structure of a child — that kind of tight coupling will always lead to breakage eventually.

The whole point of this RFC is it gives us an idiomatic way to avoid the need for those sorts of selectors in the first place.

Wouldn't treating --prop={value} as syntactic sugar for style={`--prop: ${value};`} and limit the usage to actual elements hit a nice middle ground?

Elements can already be styled. There's much less value to adding syntactic sugar for element styling than for components, where style attributes aren't a thing. That said, supporting this syntax on elements as well as components could make sense.

I feel something like this would be better

That's invalid CSS. We don't want to write our own bootleg CSS parser to handle non-standard syntax unless it's absolutely necessary, and it's not at all clear that there's an advantage here (on the contrary, making var(color) and var(--color) the same is confusing). As discussed in the RFC, using props introduces overhead (because the component has to anticipate that color might change, whereas using CSS custom properties natively allows them to be driven by even static .css files), and because the property is handled inside the component, it shadows properties declared higher up the tree.

a huge number of side effects

Don't be silly.

@sveltejs sveltejs locked as resolved and limited conversation to collaborators Nov 2, 2020
@Conduitry
Copy link
Member

This is implemented in 3.38.0 whether you like it or not. I've created #6267 for documenting this feature.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants