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

Passing CSS custom properties to components #13

Merged
merged 8 commits into from
Oct 31, 2020
Merged

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Nov 12, 2019

@rjharmon
Copy link

combining alternatives, can we recommend making the component support style= property for theming, and that authors pass styles through to whatever real dom element(s) it sees fit?

<Slider style="--rail-color: black; --track-color: red" />

@Conduitry
Copy link
Member

Conduitry commented Nov 12, 2019

Hijacking something that's already a valid prop like style would be a breaking change, and so isn't something we'd want to do know.

Actually, I guess technically this RFC is a breaking change, but the probability that someone would be using a prop starting with -- and accessing it with $$props[...] and wouldn't want that to be hijacked like this is pretty remote. I think the intuitiveness of this syntax shouldn't be prevented just because it's technically breaking.

belated edit: I see that this 'technically breaking' thing was actually already acknowledged in the RFC and dismissed, which I think is quite reasonable.

@Rich-Harris
Copy link
Member Author

I think the suggestion was for component authors to be in the habit of doing this:

<script>
  export let style = '';
</script>

<div style="{style}; color: purple">...</div>

I've added it to the Alternatives section in the RFC

@mrkishi
Copy link
Member

mrkishi commented Nov 12, 2019

I think I'm too worried about the explosion of variables, but that's what strikes me the most.

Even in the simple example used in the rfc, we're already missing at least a hover color. As in, any useable component that exposes a thing-color that's applied to something interactive will definitely need a thing-hover-color and possibly a thing-inactive-color, or the component won't be very robust.

@halfnelson
Copy link

halfnelson commented Nov 12, 2019

This is the first instance we have of a property that gets auto-applied to child dom nodes. There are a couple of other use cases for this behaviour such as, spreading slot="name" to children which would allow slot directive on components, users may want to spread a class="material" or even class="disabled" to children, or onclick=()=>some_callback_in_parent(),

Since the style is going to generate a bunch of code for each element already, could we generalize this somehow? instead of style, either have some custom prefix or prop name to specify props to be applied to dom children
eg

<Component $slot="name"/>
<!-- or  -->
<Component $childprops={{ slot: "name", class: "active", style; "--track-color: blue}}>

This would allow a more straight forward port of some other framework's components such as vue that always have a root element and passes down attributes
https://vuejs.org/v2/guide/components-props.html#Non-Prop-Attributes
https://vuejs.org/v2/guide/class-and-style.html#With-Components

@PaulMaly
Copy link

PaulMaly commented Nov 12, 2019

Not sure I like this PR more than do nothing. I don't use such kind of customizations of components in most of the cases to keep them isolated. In other cases, I use tools we already have (custom properties, special props, style forwarding etc) and don't see many problems with them. I'm sure I don't want to have additional code to be included in everyone's app.

@pngwn
Copy link
Member

pngwn commented Nov 12, 2019

Despite being personally attacked, I'm mostly on board with this rfc. The thing that strikes me the most is that it feels reversed in some ways, almost implicit. The parent decides what to pass down and the child component can use it if they want to. Control is still in the hands of the child, which is a good thing but the 'style interface' feels more implicit than explicit. Maybe this is a reasonable trade-off.

Defining an explicit style interface would be cool but I'm not sure how it would work, it would certainly mean taking an approach to styles similar to our approach in the template, CSS+ or CSSx.

Something like this in the child:

<div>
  <p><a href="" />Hi<a/></p>
</div>

<style>
  @expose { 
    --thing-color: 'blue';
    --thing-hover-color: 'pink';
    --thing-active-visited: 'red';
  };

  a {
    color: var(--thing-color);
  }

  a:hover {
    color: var(--thing-hover-color);
  }

  a:visited {
    color: var(--thing-visited-color)
  }
</style>

Which would allow only those properties to be passed in by a parent and used anywhere in the component, subsequent children would probably need to define an interface as well. This maintains encapsulation and is an explicit interface but it is definitely more limiting:

<Child --thing-color="cyan"  --thing-hover-color="magenta"/>

One question I have about the RFC, would there be a specific way to define defaults for these css properties in case one wasn't passed down? If we define them in the component directly then surely the child would overwrite the passed-down values?

@rjharmon Another issue with just using export let style = '' is that when any of those styles changed everything would be reapplied as opposed to more granular updates. Unless they were specifically handled in which case a string seems a poor choice.

@mrkishi This is true to an extent no matter what route we choose, is the specific concern just a lot of styling being done directly in the template? I figure that spreading an object of css properties could be possible, maybe this would at least improve the ergonomics.

<script>
  import Child from './Child.svelte';

  const styles = {
    "--thing-color": 'blue',
    "--thing-hover-color": 'red',
    "--thing-inactive-color": 'pink',
    "--thing-border-width": '10px',
  } 
</script>

<Child {...styles} />

@kevmodrome
Copy link

I think putting the css properties directly on the component can get crazy quite quickly. Maybe it could be done in combination with something like this (for components that require much more extensive styling):

<style component="Slider">
    .potato-slider {
        color: green;
        ...
    }
    .potato-slider-rail {
        background: rebeccapurple;
        ...
    }
</style>

The idea is to have the classes merge properties with the classes in the child thus giving the parent full control of all styles while maintaining a somewhat readable HTML.

@Rich-Harris
Copy link
Member Author

@halfnelson I worry about that being a bit of a Pandora's box — it throws encapsulation completely out the window. The nature of CSS custom properties is that they're inert unless the child chooses to do something with them, which wouldn't be the case for other things.

@pngwn

would there be a specific way to define defaults for these css properties in case one wasn't passed down? If we define them in the component directly then surely the child would overwrite the passed-down values?

Yes, in this situation the --fg property effectively isn't exposed, while the --bg property is exposed but has a fallback value, equivalent to export let bg = 'black':

<div style="--fg: red">
  ...
</div>

<style>
  div {
    color: var(--fg);
    background-color: var(--bg, 'black');
  }
</style>

@rob-balfre
Copy link

FYI svelte-select handles custom properties (CSS variables) like this...
https://svelte.dev/repl/f3bc0fd6b6f74ea499e5ecb26911bf28?version=3.12.1

@Rich-Harris
Copy link
Member Author

@kevmodrome That has the drawback of breaking encapsulation, and relying on volatile implementation details. You could still define custom properties in <style>, you'd just need a wrapper element (as you can do today):

<div>
  <Slider bind:value/>
</div>

<style>
  div {
    --rail-color: black;
    --thumb-color: red;
  }
</style>

@Rich-Harris
Copy link
Member Author

@rob-balfre perfect, it's already an ecosystem convention! (hopefully the camelCase doesn't catch on though 😛 ). So in essence, this RFC would enable the last example in that demo to change from

<style>
  .themed {
    --border: 3px solid blue;
    --borderRadius: 10px;
    --placeholderColor: blue;
  }
</style>

<div class="themed">
  <h2>Theming</h2>
  <Select {items}></Select>	
</div>

to

<h2>Theming</h2>
<Select
  {items}
  --border="3px solid blue"
  --borderRadius="10px"
  --placeholderColor="blue"
></Select>	

@kevmodrome
Copy link

@Rich-Harris

That makes sense. :)

What happens if there are 15 properties to style though (I suppose one could argue that should never happen in the first place, but still)? Wrapping the component probably is a workable solution, it just clutters up the code a bit. Personally I would want that away from the HTML (but then I'm a bit biased since I am a big fan of the styled components way of doing it).

@halfnelson
Copy link

halfnelson commented Nov 12, 2019

for completeness I am adding a comment on how this can be currently achieved with Context with downside it adds some boilerplate to components that want to be styled this way, on the up side it doesn't impact the code generated for intermediate components

Inherited

<WithStyles --rail-color="black" --trac-color="red">.
..bunch of tags nested
<Slider />
</WithStyles>

WithStyles sets a context with the style props

direct

<Slider styles="--rail-color="blue" />

in slider

<script>
   import { inheritedStyles } from 'with-styles'
   export let styles;
   $: all_styles = inheritedStyles.apply(styles)
</script>
<div class="potato-track" style={$all_styles[--track-color]} />

inheritedStyles.apply fetches styles store from WithStyles context and derives a new store based on overriding with local styles.

@Rich-Harris
Copy link
Member Author

@kevmodrome I'd expect that at that point you'd probably want those styles to be themed in a global.css file or similar — I'm anticipating this mainly being for limited, targeted overrides. But it's definitely possible that my imagination here is lacking

@rob-balfre
Copy link

@Rich-Harris ❤️🐪

Looks good but what other main benefits does it bring other than removing clutter?

I presume the compiler would output the styles as custom properties? If thats the case then postCSS can handle IE11 support.

@Rich-Harris
Copy link
Member Author

I presume the compiler would output the styles as custom properties?

Not exactly — it would apply them to top-level elements at runtime, using node.style.setProperty

@pngwn
Copy link
Member

pngwn commented Nov 12, 2019

@Rich-Harris I'm not sure about that. With many design systems there will be a vareity of themes that are applied declaratively directly in the template. I dont know what that would look like in svelte but in other libraries you might do something like:

import { theme } from './themes.js';

export default () => (
  <SystemProvider theme={theme}>
    <Thing>Hello World!</Thing>
  </SystemProvider>
);

But something similar might be possible with the RFC proposal.

@rob-balfre
Copy link

I presume the compiler would output the styles as custom properties?

Not exactly — it would apply them to top-level elements at runtime, using node.style.setProperty

Hmm IE11 support is a must have for us - wish it wasn't but 30% of our clients (big corps etc) are still stuck on it. So with your approach you couldn't emit the CSS with the custom properties included?

@Rich-Harris
Copy link
Member Author

The CSS would reference the custom properties, but they wouldn't have values (since those are set at runtime). IOW you'd be left with a) the fallback values, or b) whatever your preprocessor replaced the custom property references with (which is a totally legitimate solution that I think we should encourage, in situations where you know the values ahead of time)

@korywka
Copy link

korywka commented Nov 13, 2019

If root component doesn't define hover, active, focus style properties (color, text-decoration, opacity, ...) we can't add them just with the help of CSS constants. Theming is not just changing colours, it is ability to re-define or define new CSS properties. Noting about that in rfc.

@korywka
Copy link

korywka commented Nov 13, 2019

@Rich-Harris for more flexible solution something like that:

<script>
   import Slider from './slider.svelte';
</script>

<Slider />

<style data-scope="Slider">
   .rail { color: blue; }
   .rail:before { content: ''; display: block; ... }
   .track:hover { opacity: 0.5; }
</style>

under the hood it works like Object.assign: {...root styles, ...theme styles}

@takoyaro
Copy link

This is clever and is something I've been hoping for.

<Component --rail-color="rgba(20,122,255,0.8)"/>

Juste makes so much sense. 
I don't need anything fancy, just this is fine.

@rob-balfre
Copy link

The CSS would reference the custom properties, but they wouldn't have values (since those are set at runtime). IOW you'd be left with a) the fallback values, or b) whatever your preprocessor replaced the custom property references with (which is a totally legitimate solution that I think we should encourage, in situations where you know the values ahead of time)

Currently for IE11 support we use https://github.com/postcss/postcss-custom-properties
but, after this change, would this do anything useful during Svelte preprocess?

@rob-balfre
Copy link

TIL I learnt about CSS Houdini ... https://youtu.be/-oyeaIirVC0?t=894 (Chrome dev conference)

@pngwn
Copy link
Member

pngwn commented Aug 4, 2020

Our position on using CSS and classes directly to solve these issues has been clarified here. It has not changed in the past week and we don't want to keep discussing the same thing again and again.

@Zachiah
Copy link

Zachiah commented Aug 11, 2020

I just had an idea, what is the issue with something like this.

Button.svelte

<button svelte:styles><slot></slot></button>
<p class="tooltip" svelte:styles="tooltip"><i svelte:styles="i">tool</i>tip</p>

Consumer.svelte

<Button>Hi</Button>

<script>
   import Button from "./path/to/Button.svelte";
</script>

<style>
    Button {
        color: blue;
        background: red;
        ...
    }
    Button:tooltip { /* not necessarily an exact syntax, but something like this */
        background: orange;
    }
</style>

In that way component authors could specify exactly what parts of components are allowed to be customized. From the compilers perspective it would replace

Button {
 ...
}

with a selector that only selects what it's supposed to like

.svelte-foo {
  ...
}

This also allowes better css like

input + Button:tooltip {...} 

or whatever.

You can only style subelements if they are explicitly enabled by the child. i.e.

Button:tooltip i {} /* Doesn't work */
Button:tooltip Button:i /* Does work because I enabled it in the markup */

Also if you want an elements children to all be enabled for styling there could be a special attribute for it. i.e.

<p svelte:styles="theP" svelte:styles:children={true}><!-- TONS OF ELEMENTS HERE --></p>

I really like this approach. Let me know if there are any issues, or something I'm missing.

@sveltejs sveltejs locked as resolved and limited conversation to collaborators Aug 11, 2020
@Rich-Harris Rich-Harris mentioned this pull request Oct 31, 2020
@Rich-Harris Rich-Harris merged commit fbcc259 into master Oct 31, 2020
@Rich-Harris Rich-Harris deleted the style-properties branch October 31, 2020 17:05
@sveltejs sveltejs unlocked this conversation Oct 31, 2020
@TylerRick
Copy link

Since the style-properties branch got deleted, the link in the description is now broken.

Should we update the description to point to new URL?:
https://github.com/sveltejs/rfcs/blob/master/text/0000-style-properties.md

@Zizico2
Copy link

Zizico2 commented Nov 17, 2020

Replying to "I'm not a big design system user, so I would very much like to get feedback from people who are. Would this solve your problems? What have we missed?"

I still have a problem with styling elements that this doesn't solve. This problem is not strictly styling since you might run into this for other reasons. I'm trying to implement a material ripple. My goal is to make this ripple component completely stylable by the consumer. The easiest solution is to just forward the styles attribute of my top component. It works well but then the consumer can't use classes to style the component, they have to inline everything, and cant use any attribute of the component, although they could just wrap it with a div or a span for that. My current solution is to use:an action. So the consumer can import my ripple styling globally and do <div class="my-custom-style" use:ripple \> wherever they want. This solution is quite nice to use tbh, from a consumer's point of view. But it comes with the downside that I need to implement my ripple imperatively and lose all the benefits of svelte, and svelte can't optimize the css. The first thing that comes to mind is to handle "class" as a special prop (I know noone likes the idea xd), but all the other attributes would also need to be treated as special cases (although, again, for those the consumer could just wrap the div). Another solution would be for <slot/ > to alias to the top element in it, if there is only one, (I believe there is already a closed issue on this on the svelte repo, although I couldn't find it) or to introduce a new element, <target /> that behaves like <slot /> but only allows one child (and therefore could always alias to it). That way I could use a component instead of an action: <Ripple><div class="my-custom-style" \></Ripple>.
Another wild idea I had was for there to be a way to write actions declaratively, very much like a component, that could make use of a <target /> component, since actions can only be applied to a single element, and not components.

@PaulMaly
Copy link

PaulMaly commented Dec 26, 2020

Sorry guys, maybe I would be silly again, but I want to be sure we really need this RFC in the way it has been proposed:

REPL

Or maybe, we can get the same feature much cheaper and explicit in user-land without any core-changes already now?

Of course, even in this way a huge number of side-effects still will be there, but at least it will be explicit:

REPL

@hperrin
Copy link

hperrin commented Apr 21, 2021

As the author of a UI library, I would say this is unnecessary as per @PaulMaly's design. That being said, I would love to be able to target a component with a class name without relying on something like this:

<div>
  <MyComponent class="my-class-name" />
</div>

<style>
  * :global(.my-class-name) {
    width: 200px;
  }
</style>

First of all, I have to put the Component under an element, or I can pollute things outside of my component with the same class name. Second, if MyComponent renders anything underneath it that uses the "my-class-name" class internally, it also receives the styles. Third, it is super confusing for new users, and a frequent pain point for onboarding users to my (or I assume anyone's) UI library.

@non25
Copy link

non25 commented Apr 21, 2021

Solution! Is it visible enough?

We can just end this suffering at the cost of scoping for non-class selectors and meet the consequences of class-passing, whatever it might be.

There's a cssModules scoping preprocessor, which allows to:

  • write class="some-class" instead class={css.someClass}
  • use the style tag
  • pass hashed classes to components without div wrappers
  • be safe of inheriting outer site styles because we used some generic .modal class in our widget-app
  • avoid selector weight wars

Try it

Install it:

npm i -D svelte-preprocess-cssmodules svelte-as-markup-preprocessor

Adjust preprocess config in the bundler:

const cssModules = require('svelte-preprocess-cssmodules');
const sveltePreprocess = require('svelte-preprocess');
const { asMarkupPreprocessor } = require('svelte-as-markup-preprocessor');
// ...
preprocess: [
  asMarkupPreprocessor([
    sveltePreprocess()
  ]),
  cssModules()
],
// ...

Use it:

<button class="btn {$$restProps.class || ''}"><slot /></button>
<!-- something else below -->
<div>
  <p class="future-is-bright">The future is bright</p>
  <Button class="margin-top">Click me!</Button>
</div>

<style lang="scss" module>
  .future-is-bright {
    color: bright;
  }

  .margin-top {
    margin-top: 6px;
    // whatever you want
  }
</style>

Preprocessor readme

dummdidumm pushed a commit to dummdidumm/language-tools that referenced this pull request Apr 24, 2021
Remove --custom-css-propeties during Component attribute transformations. They have no property which they can be checked against and the leading dashes are invalid JSX

Related RFC: sveltejs/rfcs#13
dummdidumm added a commit to sveltejs/language-tools that referenced this pull request Apr 24, 2021
Remove --custom-css-propeties during Component attribute transformations. They have no property which they can be checked against and the leading dashes are invalid JSX

Related RFC: sveltejs/rfcs#13
@bluwy bluwy mentioned this pull request May 26, 2021
@samhsabin
Copy link

In 2024, is there documentation on the best practice usage of what is currently implemented? I want to use a theming system but I'm a bit confused on what the final say for this issue was, and what that looks like in practice. Tutorials online from third parties all use different methods.

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.

None yet