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

Style directives #42

Merged
merged 6 commits into from
Nov 29, 2021
Merged

Style directives #42

merged 6 commits into from
Nov 29, 2021

Conversation

plmrry
Copy link
Contributor

@plmrry plmrry commented Dec 9, 2020

@swyxio
Copy link

swyxio commented Dec 9, 2020

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

@Rich-Harris
Copy link
Member

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
Copy link

opensas commented Dec 10, 2020

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
Copy link

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
Copy link

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

@pngwn
Copy link
Member

pngwn commented Dec 10, 2020

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
Copy link
Member

antony commented Dec 10, 2020

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
Copy link
Member

pngwn commented Dec 10, 2020

@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
Copy link

pushkine commented Dec 10, 2020

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

  • the property name formatting is unclear
  • the directive clashes with the attribute
  • each property requires an extra 9 characters boilerplate style:={}

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
Copy link

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
Copy link
Member

antony commented Dec 10, 2020

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
Copy link
Member

pngwn commented Dec 10, 2020

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
Copy link
Member

antony commented Dec 10, 2020

<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
Copy link

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
Copy link

MarkTanashchuk commented Dec 10, 2020

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
Copy link

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

@ivanhofer
Copy link

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
Copy link

lukaszpolowczyk commented Dec 10, 2020

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!
</span>
  1. Svelte modifiers:
<script>
  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>

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:

<script>
  let color = 'red';
  let fontSize = 16;
</script>

<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!
</span>

@codymikol
Copy link

codymikol commented Dec 10, 2020

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
Copy link
Member

antony commented Dec 10, 2020

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
Copy link
Member

pngwn commented Dec 12, 2020

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
Copy link

Monkatraz commented Dec 12, 2020

<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
Copy link

<!-- 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
Copy link
Member

pngwn commented Dec 12, 2020

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

@lukaszpolowczyk
Copy link

@pngwn Who are you addressing?

@pngwn
Copy link
Member

pngwn commented Dec 12, 2020

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
Copy link

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
Copy link
Member

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
Copy link
Contributor Author

plmrry commented Dec 14, 2020

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.

@plmrry plmrry mentioned this pull request Jan 24, 2021
31 tasks
@milkbump
Copy link

milkbump commented Feb 20, 2021

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
Copy link
Contributor Author

plmrry commented Feb 21, 2021

@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
Copy link

milkbump commented Feb 21, 2021

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
Copy link

@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>

@antony antony mentioned this pull request Mar 18, 2021
2 tasks
@madeleineostoja
Copy link

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
Copy link
Member

merging, since #5923 is good to go

@Rich-Harris Rich-Harris merged commit ca46eb3 into sveltejs:master Nov 29, 2021
@opensas
Copy link

opensas commented Nov 30, 2021

is this ready to give it a try?

@nosovk
Copy link

nosovk commented Dec 4, 2021

is this ready to give it a try?

sveltejs/svelte#5923
Watch that PR, it will be right after merge I hope

@janosh
Copy link

janosh commented Jan 14, 2022

What was the final verdict on camel casing?

<div style:maxWidth />

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

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