#666 support css variables

[tkh44]

This is most likely unacceptable as far as size goes, but I wanted to get the process started.

Note that PhantomJS does not support cssText with custom properties in it. No idea what to do about this other than to drop css vars from that test.

fixes #666

[coveralls]

Coverage remained the same at 100.0% when pulling 8a8e685 on tkh44:666-support-css-variables into 0dea3b7 on developit:master.

[developit]

I think we can always use setProperty() / removeProperty() here, which saves a bunch of bytes. AFAIK style.setProperty('color', 'red') is equivalent to style.color='red'.

[developit]

just re-read the original thread, i was wrong since setProperty() uses CSS case rather than camelCase. poop! one option would be if (~i.indexOf('-')), which might be neat since it also enables support for css-case in general.

[lukeed]

Damn GitHub collapsing comments. Missed this

[developit]

Awesome.

Regarding the testing, it's probably a good time to switch the default launcher to be Headless Chrome, I don't really use Phantom for Preact anymore since the performance numbers are unreliable.

[coveralls]

Coverage remained the same at 100.0% when pulling 7273c81 on tkh44:666-support-css-variables into 0dea3b7 on developit:master.

[developit]

the bundle size thing didn't kick in, but it looks like this goes from 3444 -> 3482 (+38b). Honestly that's not too bad, and I really want CSS variables support.

One size thing I thought of - do you think style.setProperty('--foo', '') would work like style.removeProperty('--foo')? Would cut a few bytes after gzip.

[tkh44]

do you think style.setProperty('--foo', '') would work like style.removeProperty('--foo')?

According to MDN this should work style.setProperty('--foo'), but it errors in chrome. style.setProperty('--foo', '') works fine though and is mentioned in the spec: If value is the empty string, invoke removeProperty() with property as argument and terminate this algorithm so 👍

[coveralls]

Coverage remained the same at 100.0% when pulling b98bc18 on tkh44:666-support-css-variables into 0dea3b7 on developit:master.

[coveralls]

Coverage remained the same at 100.0% when pulling b2c742e on tkh44:666-support-css-variables into 73866a4 on developit:master.

[jveyssie100614]

What about using the ternary operator ?

	 ~i.indexOf('-')
		 ? node.style.setProperty(i, value[i]) 
		 : node.style[i] = typeof value[i]==='number' && IS_NON_DIMENSIONAL.test(i)===false 
			? (value[i]+'px') 
			: value[i];

[lionralfs]

@JeremyVSR pretty sure it gets minified like that anyways.

But I think removing the ===false check and flipping the ternary on the Regex check might save a few bytes.

[lukeed]

Untested -- assumes everything can be set via setProperty, which looks to be the case at quick glance.

[lukeed]

Challenge: https://twitter.com/preactjs/status/998981752130588673

if (value && typeof value==='object') {
  let i; // placeholder
  if (typeof old!=='string') {
    for (i in old) {
      if (!(i in value)) node.style.setProperty(i, '');
    }
  }
  for (i in value) {
    node.style.setProperty(i, typeof value[i]==='number' && IS_NON_DIMENSIONAL.test(i)===false ? (value[i]+'px') : value[i]);
  }
}

[PepsRyuu]

This would be backwards incompatible. Doesn't work for properties that are defined using camel-casing.

node.style.backgroundColor = value; // works
node.style['background-color'] = value; // works
node.style.setProperty('background-color', value); // works
node.style.setProperty('backgroundColor', value); // doesn't work

[lukeed]

@PepsRyuu aye, saw this after 🙈 /delete

[andrewiggins]

@tkh44 Do we need the if checks at all? I removed the if statements (shown below) and the PR size was reduced by 17 bytes, so its size is now only +9 bytes on top of master. All tests pass. I quickly checked in a handful in browsers and I think this should work. It appears setProperty doesn't throw an error on invalid property names (which will happen if a component defines something like backgroundColor like you've pointed out).

if (value && typeof value==='object') {
	if (typeof old!=='string') {
		for (let i in old) if (!(i in value)) {
			node.style.setProperty(i, '');
			node.style[i] = '';
		}
	}
	for (let i in value) {
		node.style.setProperty(i, value[i]);
		node.style[i] = typeof value[i]==='number' && IS_NON_DIMENSIONAL.test(i)===false ? (value[i]+'px') : value[i];
	}
}

[tkh44]

@andrewiggins that should work. There was a bug in the older code that I just realized.

typeof value[i] ===
				'number' &&
				IS_NON_DIMENSIONAL.test(i) === false ? (value[i] + 'px') : value[i])

needs to be in both the setProperty and the assignment.

abstracting it would look like

function a(b) { typeof b ===
				'number' &&
				IS_NON_DIMENSIONAL.test(i) === false ? (b + 'px') : b)

Not sure where this puts us in terms of bytes.

[developit]

We should check what the size is when doing if (i[0]=='-') - we only care about using setProperty for custom properties, so it'll always be a leading --.

[marvinhagemeister]

Closing, we finally have support for css variables in Preact X 🎉💯