sveltejs / rfcs

RFCs for changes to Svelte
275 stars 82 forks source link

Style directives #42

Closed plmrry closed 2 years ago

plmrry commented 3 years ago

Rendered

swyxio commented 3 years ago

pretty interesting. i wonder what other attributes than style and class this is worth extending to.

Rich-Harris commented 3 years ago

I like this and think we should do it.

Re conflicts, style: directives should definitely take priority I think. In the common case...

<div style="color: {color}; font-size: {size}px" style:color="blue">

...we can and do extract the individual color and font-size properties, making conflict detection easy. I don't see any reason that shouldn't be a compile error. The trickier case is

<script>
  export let color;
  export let size;
  $: styles = `color: ${color}; font-size: ${size}px`;
</script>

<div style="{styles}" style:color="blue">

since we have to bail out of the optimisation and resort to setting cssText; if we had directives as well then we would have to reset all the directive-based styles immediately after setting cssText, which is unfortunate. Options are

a) deal with it — we're already bailing out of the optimisation b) disallow a combination of cssText-requiring styles and style directives

a) is probably more philosophically consistent with the rest of Svelte, though perhaps it should generate a warning.

Personally I'm inclined not to do the camelCase thing, because we don't do that anywhere else, we force you to be explicit:

<div class:is-active={isActive}>

It could be argued that styles are different because these are actual properties of node.style rather than made up class names, but I still think it's probably better to be consistent and learnable even at the cost of occasionally requiring a few extra keystrokes.

opensas commented 3 years ago

I wonder if we could also support react style directive, that is to also support something like:

const style = { marginLeft: "30px", backgroundColor: "red" }
</script>
[...}
<div style={style} />

or also the common shortcut <div {style} />

I'm not so sure if both syntax might be compatible...

sidx1024 commented 3 years ago

It's a good idea. I made a custom preprocessor sometime back to support style directive.

https://github.com/sidx1024/svelte-style-directive https://www.npmjs.com/package/svelte-style-directive

stephane-vanraes commented 3 years ago

How would we get this working with custom css properties ? style:--base="12px" doesn't even look like it would be valid

pngwn commented 3 years ago

I wonder if we could also support react style directive, that is to also support something like:

const style = { marginLeft: "30px", backgroundColor: "red" }
</script>
[...}
<div style={style} />

@opensas This isn't relevant to this RFC.

antony commented 3 years ago

How would we get this working with custom css properties ? style:--base="12px" doesn't even look like it would be valid

Looks ok to me: https://svelte.dev/repl/b862199691df49e9b2e9c68c707f249b?version=3.31.0

pngwn commented 3 years ago

@stephane-vanraes @antony Yeah it is valid syntactically. Even if it weren't, we could explicitly support it as we control the parser and most of our tooling.

pushkine commented 3 years ago

I think we can all agree that setting style properties dynamically through raw css strings is unpleasant

With that said the proposed solution has minor drawbacks

I'd suggest going for something much easier, that would be for the style attribute to simply support inline object literals as an input, thus enabling the following syntax

<div style={{ 'padding-left': (offset + 2) * 16 + 'px' }} />

To make it easy for everyone, here's a dirt cheap way to also support camelCased properties

if (property in node.style) node.style[property] = value;
else node.style.setProperty(property, value)

On a side note I would like to underline that inline style is better left to dynamic styling, as all examples in the RFC are either constant or most fit for the class directive

stephane-vanraes commented 3 years ago

I think we can all agree that setting style properties dynamically through raw css strings is unpleasant

each property requires an extra 9 characters boilerplate style:={}

I think the strength of this proposal lies in making it easier for conditional properties

style={padding ?padding: ${padding}: ''}

vs

style:padding

antony commented 3 years ago

I'd suggest going for something much easier, that would be for the style attribute to simply support inline object literals as an input, thus enabling the following syntax

<div style={{ 'padding-left': (offset + 2) * 16 + 'px' }} />

You don't need an RFC for that, and it's not easier to read than:

<div style:padding-left={(offset + 2) * 16 + 'px'}>

each property requires an extra 9 characters boilerplate style:={}

it's only boilerplate if you require it. It's syntactic sugar, and it's optional, if you want to specify a dynamic style property

the directive clashes with the attribute

I disagree, because style: is a namespace, and style is an attribute.

pngwn commented 3 years ago

I don't think the clash will have a huge impact because the same is true of class attributes and directives and that is a very popular construct that gets used extensively and often together. There is an issue of precedence but that has already been mentioned.

antony commented 3 years ago
<div style={{ 'padding-left': (offset + 2) * 16 + 'px' }} />

just to clarify about not needing an RFC:

https://svelte.dev/repl/c7787db098e6495888036598586ec4f3?version=3.31.0

I did originally thing just passing and reading a style prop would be fine but then I realised that will only work for components.

stephane-vanraes commented 3 years ago

How would we get this working with custom css properties ? style:--base="12px" doesn't even look like it would be valid

Looks ok to me: https://svelte.dev/repl/b862199691df49e9b2e9c68c707f249b?version=3.31.0

@stephane-vanraes @antony Yeah it is valid syntactically. Even if it weren't, we could explicitly support it as we control the parser and most of our tooling.

That was badly expressed, the problem is, again, the combination with conditionals. This construct would be very nice to pass on custom variables:

<script>
  export let mycustomprop
</script>

<div style:--mycustomprop={mycustomprop}>

In this case the shorthand would not work because --mycustomprop is not a valid name for a variable. But then again, it can still be done as I write above, so there would only be need to have a note with this caveat.

MarkTanashchuk commented 3 years ago

What do you think about an idea of allowing to pass styles to a component, and, inside it, set styles to tags, like you can do now with events (e.g., with on:click):

//App.svelte
<SomeComponent style:transform={"translate(20px, 20px)"} />
<AnotherComponent on:click={() => console.log("Clicked")}/>
// SomeComponent.svelte
<div style>Text</div>
//style:all ?
//style:transform ?
//AnotherComponent.svelte
<div on:click>Click me</div>
MarkTanashchuk commented 3 years ago

It seems to me that with this implementation it can be done...

ivanhofer commented 3 years ago

It would also be nice if the tags would contain validation where possible. A warning could be displayed if someone passes a wrong attribute e.g.

<script>
   export let top
   const position = top ? 'top' : 'middle' 
</script>

<div style:position>
  ...
</div>

Warning: the property 'middle' has no effect on the 'position' style attribute.

lukaszpolowczyk commented 3 years ago

What with CSS Typed Object Model?

With Svelte modifiers, it could look like this: style:font-size|px={fontSize}

Props:

  1. Object window.CSS:
<script>
  const {px} = CSS;
  let color = 'red';
  let fontSize = 16;
</script>

<span
  style:color={color}
  style:font-size={px(fontSize)}
  style:font-weight="bold"
  style:text-transform="uppercase">
  Hi there!
</span>
  1. Callback:
    
    <script>
    let color = 'red';
    let fontSize = 16;
    </script>

<span style:color={color} style:font-size={({px})=>px(fontSize)} style:font-weight="bold" style:text-transform="uppercase"> Hi there!

3. Svelte modifiers:

<span style:color={color} style:font-size|px={fontSize} style:font-weight="bold" style:text-transform="uppercase"> Hi there!


The first and second propositions might look like this: `fontSize|>px` with JS pipeline operator.

I do not hide that the third proposition seems to be the most interesting. :)

---

Additionally - Support for more than one property:

<span style:color={color} style:font-size|px={fontSize} style:text-shadow|px|px|color={[2, 2, '#FF0000']}; style:font-weight="bold" style:text-transform="uppercase"> Hi there!

codymikol commented 3 years ago

This could be a really nice way to make CSS transforms easier to work with. We could parse the resulting css under the hood.

.foo {
  transform: rotate(15deg) transalate(-20px,0px)
}

could become

<div 
  style:transform-rotate={'15deg'}
  style:transform-translate={'-20px 0px'} 
/>

That would be really neat for making them dynamic, and maybe there can be syntactic sugar for dealing with certain units.

<div 
  style:transform-rotate-deg={calculateRotation()}
  style:transform-translate-y={calculateY()} 
/>
antony commented 3 years ago

This could be a really nice way to make CSS transforms easier to work with.

<div 
  style:transform-rotate={'15deg'}
  style:transform-translate={'-20px 0px'} 
/>

We would really be building ourselves a maintenance headache there. Every time the CSS spec changes we'd need to add a bunch of transformations.

It does indeed look nice, but i would keep it simple to avoid a lot of conversion and mysterious syntax which would require documentation:

let value = '15px'
$: transform = `rotate(${value})`

<div style:transform={transform}></div>
pngwn commented 3 years ago

Any syntax here needs to have a 1-2-1 mapping to the CSS spec as much as possible. This should be a generic abstraction on top of CSS that makes learnability straightforward by following a simple pattern. Lots of complex exceptions would make this more difficult to learn and more difficult to maintain over time.

Monkatraz commented 3 years ago

<span style:text-shadow|px|px|color={[2, 2, '#FF0000']}; />

I do love the terse-ness and readability of this, but what happens if you do this?

<!-- is this a syntax error? would using the directive modifiers imply an array input? -->
<span style:text-shadow|px|px={2}; />

Also, would using |px simply do input + 'px' or would it type-check the input? Would you have to add every single unit type or would the modifier's written name be used as the unit? EDIT: Just realized I missed the link to the typed object model thingy, so nvm. Still, this is a bit finicky in some cases.

You can get something fairly readable just by using template strings:

<span style:text-shadow={`${2}px ${2}px #FF0000`}; />

EDIT: Also, this doesn't allow for a syntax that would allow you to dynamically alter the CSS unit type of the input you're feeding in, unfortunately.

lukaszpolowczyk commented 3 years ago
<!-- is this a syntax error? would using the directive modifiers imply an array input? -->
<span style:text-shadow|px|px={2}; />

Rather, a warning is enough. In this case, the first unit will be used.

pngwn commented 3 years ago

Further to my previous comment, this discussion is basically why we would never implement this kind of syntax.

lukaszpolowczyk commented 3 years ago

@pngwn Who are you addressing?

pngwn commented 3 years ago

Anyone discussing syntax that doesn't observe this rule:

Any syntax here needs to have a 1-2-1 mapping to the CSS spec as much as possible. This should be a generic abstraction on top of CSS that makes learnability straightforward by following a simple pattern. Lots of complex exceptions would make this more difficult to learn and more difficult to maintain over time.

codymikol commented 3 years ago

This could be a really nice way to make CSS transforms easier to work with.

<div 
  style:transform-rotate={'15deg'}
  style:transform-translate={'-20px 0px'} 
/>

We would really be building ourselves a maintenance headache there. Every time the CSS spec changes we'd need to add a bunch of transformations.

It does indeed look nice, but i would keep it simple to avoid a lot of conversion and mysterious syntax which would require documentation:

let value = '15px'
$: transform = `rotate(${value})`

<div style:transform={transform}></div>

True, seeing examples using template strings seems to be less verbose anyway.

<div 
  style:transform-rotate={'15deg'}
  style:transform-translate={'-20px 0px'} 
/>

becomes


<div style:transform={'rotate(${rotation}deg) translate({x}px ${y}px)'}/>
Rich-Harris commented 3 years ago

You don't even need template strings — instead of

<div style:transform={`rotate(${rotation}deg) translate(${x}px ${y}px)`}/>

you can do

<div style:transform="rotate({rotation}deg) translate({x}px {y}px)"/>
plmrry commented 3 years ago

On the topic of accepting object literals:

While I'd personally be happy if the style attribute accepted an object literal, I don't think we should do it. To steal a phrase from @Rich-Harris above, using object literals for style attributes seems less "philosophically consistent with the rest of Svelte." While merging and passing down style objects may make sense in certain scenarios, it does not seem like a best practice that Svelte would want to encourage by default.

I'd also note that if one does want to construct style attribute strings from object literals, a relatively simple helper function can be used (for example here):

<script>
  import styleToCss from 'style-obj-to-css-string';

  export let maybeNull = null;
  export let width = 40;
  export let coolVariable = "goldenrod";
  export let otherStyle = {};

  $: style = styleToCss({
    position: maybeNull && "absolute",
    width: `${width}px`,
    "--my-cool-variable": coolVariable,
    ...otherStyle
  });
  // `style` is: "width: 40px; --my-cool-variable: goldenrod;"
</script>
<div {style}>hello</div>

In any case, it's something that could be discussed in a separate RFC, as it's not mutually exclusive with the current proposal.

milkbump commented 3 years ago

My main issue with this is that many dynamic styles become directive soup. What do you think about compiling to CSS variables (custom properties) as opposed to style directives?

<script>
  export let maybeNull = null;
  export let width = 40;
  export let coolVariable = "goldenrod";
</script>
<div>
    hello
</div>

<style>
     div {
          position: {maybeNull && "absolute"};
          width: {width (100)}px; /* 100px fallback? */
         --my-cool-variable: {coolVariable};
     }
</style>

which would compile to have equivalent behaviour to custom properties:

<script>
  export let maybeNull = null;
  export let width = 40;
  export let coolVariable = "goldenrod";
</script>
<div 
style="--svelte-hash1: {maybeNull && "absolute"}; --svelte-hash2: {width}px; --my-cool-variable: {coolVariable};>
    hello
</div>

<style>
     div {
          position: var(--svelte-hash1); /* Ignored if blank */
          width: var(--svelte-hash2, 100px /* fallback */); 
     }
</style>

Dynamic styles can then be reused more easily than with a style: directive. Though, this would probably not work for preprocessing SASS and friends.

Maybe this warrants an RFC of its own.

plmrry commented 3 years ago

@kwangure allowing JS variables in CSS would be a rather huge paradigm shift — and the idea of mixing JS and CSS (even if it's only at the compiler level) is generally discouraged as going against the philosophical grain of Svelte.

milkbump commented 3 years ago

On the contrary, it feels so well aligned with Svelte's philosophy to me. Custom properties enable shifting dynamic and repeated CSS values without messing with CSS. Svelte could make this even easier.

And yes, it's a paradigm shift —but that's a good thing. "Paradigm shift" characterizes many features I like about Svelte.

My only concern with my suggestion is the feasibility. A svelte CSS block (and by extension custom properties) is shared among several component instances, but changing state in one component instance should not affect the styling of another downstream. If it were possible to solve this through some compiler magic (I don't have ideas yet), then I think this definitely feature is up Svelte's wheelhouse.

That said, my concern here was around having a div littered with a chain of style:property={value? "string": "different-string"} directives. It feels repetitive and noisy. Suggestions for making it cleaner are welcome. Otherwise, it may be a necessary compromise if consensus for style directives is reached.

stephane-vanraes commented 3 years ago

@kwangure using JS variable in the CSS will also make it (nearly) impossible to extract the CSS to it's own file without adding a fair amount of complexity related to updating those styles in the compiled code, probably not entirely impossible. But as you say their still will need to be a system to differentiate instances from each other. The "easiest" way to this is to then lift out the css properties that use js variables during compilation and inline them (basically have the compiler do the littering)

Perhaps a precompiler could do this for you instead

<script>
  let height = 100
</script>

<div>Test</div>

<style>
  div {
    color: red;
    height: {height}px;
  }
</style>

would then be 'precompiled' to

<script>
  let height = 100
</script>

<div style="height: {height}px">Test</div>

<style>
  div {
    color: red;
  }
</style>
madeleineostoja commented 3 years ago

I seem to be running against the crowd here, but this feels like unnecessary sugar to me. Inline styles are already well established, and with Svelte's existing string templating conditionals in them behave consistently and as expected. This just introduces yet another API to the Svelte DSL for new developers to become accustomed to, and can lead to messy style: declarations littered throughout a component's attributes.

Other than the saved keystrokes, is there any practical benefit to this RFC that I'm missing?

Rich-Harris commented 2 years ago

merging, since #5923 is good to go

opensas commented 2 years ago

is this ready to give it a try?

nosovk commented 2 years ago

is this ready to give it a try?

https://github.com/sveltejs/svelte/pull/5923 Watch that PR, it will be right after merge I hope

janosh commented 2 years ago

What was the final verdict on camel casing?

<div style:maxWidth />

Not supported and won't be coming in the future?