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

Add support for CSS Custom Properties #666

Closed
davidkpiano opened this issue Apr 24, 2017 · 10 comments
Closed

Add support for CSS Custom Properties #666

davidkpiano opened this issue Apr 24, 2017 · 10 comments

Comments

@davidkpiano
Copy link

Cross-references with React:

React will soon be able to support CSS Custom Properties (CSS Variables) by using .style.setProperty(<prop>, <value>) instead of directly modifying .style.<prop> = <value> for custom properties (detected by seeing if the property starts with two dashes, e.g., --foo-bar).

It seems that, for objects passed into the style attribute of Preact components, this is currently not the case, as it is still directly setting the style properties: https://github.com/developit/preact/blob/9e1dd185bb1d6505c35477d8101c886c25dc51c8/src/dom/index.js#L57

Furthermore, the use of .style.setProperty(prop, value) seems to be much faster in most environments (except for IE9-11, where it is slightly slower): https://jsperf.com/dom-style-vs-setproperty - shouldn't matter anyway if you choose to use .setProperty for only custom properties.

The end-goal is to have this work:

<div style={{ '--foo': 'green', color: 'var(--foo, red)' }}> 
  ...
</div>

What do you think? 😄 It's a fairly minor change and I can make a PR if needed.

@developit
Copy link
Member

developit commented Apr 25, 2017

Ah yeah I've been meaning to investigate this! Since this is Preact and every byte is slaved over, what do you think about always using style.setProperty() on the line you linked to?

- node.style[i] = /* snip */ value[i];
+ node.style.setProperty(i, /* snip */ value[i]);

It seems like that would enable CSS Custom Properties and even improve perf at the same time, and the cost would be really small. Thoughts?

(edit: presumably we'd need to switch style.foo='' to style.removeProperty('foo') too?)

@davidkpiano
Copy link
Author

I think it's a 💯 good idea - there's only one downside that the React team ran into - you would have to use the normal, hyphenated CSS property names instead of the camelCase property names:

// this won't work
node.style.setProperty('backgroundColor', 'green');

// this will work
node.style.setProperty('background-color', 'green');

This means that when passing in styles as objects, such as:

<div style={{ backgroundColor: 'green' }} />

you'd have to do some mapping or normalization of the property name to get the hyphenated name. Or you can just always require the hyphenated name -- it would just be a breaking change, though.

@developit
Copy link
Member

developit commented Apr 26, 2017

ahhh interesting... probably have to keep react compat there.
you wouldn't happen to know if style.cssText supports custom properties would you?

node.style.cssText = '--foo: green; background-color: var(--foo, red);'

@davidkpiano
Copy link
Author

It does on Chrome, FireFox, Safari (haven't tested Edge yet)

@nicolasparada
Copy link

Does work on Edge.

@developit
Copy link
Member

Interesting - so it's possible serializing to cssText could solve this. I was considering switching back to serialization in order to lighten up the object diff we currently have to do, which is theoretically faster because it sets single properties, but in reality the object traversal cost seems to outweigh any benefit.

@tkh44
Copy link

tkh44 commented Aug 22, 2017

Is there any way I can help on this @developit? If you tell me which strategy to take I'd be happy to open a PR.

@davidkpiano
Copy link
Author

@tkh44 It might be safest to use the same implementation that Facebook used (at least for starters): facebook/react#9302

@benjamminj
Copy link

benjamminj commented Dec 2, 2017

@developit any news on this feature? I saw the PR got out of date with master...

If there's anything I can do to help would love to as I just found myself looking for this feature/wishing I could set variables inline this morning

@marvinhagemeister
Copy link
Member

This feature has just landed in master via #1302 🎉

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

Successfully merging a pull request may close this issue.

6 participants