WebReflection / hyperHTML

A Fast & Light Virtual DOM Alternative
ISC License
3.07k stars 113 forks source link

Support CSS custom properties in style object #271

Closed jiayihu closed 6 years ago

jiayihu commented 6 years ago

Here I am again, hopefully not wasting your time again.

This PR should allow usage of CSS custom properties when the style is passed as object. Currently it works only if passed as string. Codepen.

Unfortunately the new test case I'm trying to add doesn't work because setProperty and getPropertyValue are missing in CSSStyleDeclaration. I'm not familiar with the test system used here, although I think jsdom is used and maybe it's related to https://github.com/jsdom/jsdom/issues/1895 .

Also not sure if repeating the same code in index.js, esm/ and cjs/ is correct. At a quick look of past commits and package.json, none of the three seem to be generated automatically.

TLDR; the fix should be quick, but there are problems with the build system which I'm not familiar to. If it's quicker for you, I'm fine with closing the PR and leave it to you rather than explaining the details of how build/tests are set up in the repo.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at ?% when pulling 5b59935defdf47a0dd680da1549915b5fa742cd7 on jiayihu:css-custom-properties into 4faba8dc096155e8ddbbb05ce1e0b53cd7225f9e on WebReflection:master.

WebReflection commented 6 years ago

Here I am again, hopefully not wasting your time again.

You do that only if you don't read what I write 😜

This PR should allow usage of CSS custom properties when the style is passed as object. Currently it works only if passed as string.

good point, I haven't used style too much myself but I think this might be helpful.

I'm not familiar with the test system used here, although I think jsdom is used

Nope, not at all. It's basicHTML 'cause jsdom doesn't support (didn't?) Custom Elements and it's too strict with the standard + slower and bloated for my needs.

However, I might need to implement both setProperty and getPropertyValue in there before moving on here.

Also not sure if repeating the same code in index.js, esm/ and cjs/ is correct.

The source of truth is the esm/* folder, then you simply npm run build and everything happens.

Since coveralls is broken, but this repo has effectively 100% code coverage, I won't merge until that very same percentage is reached.

none of the three seem to be generated automatically.

npm-dollar does everything for me 🎉

If it's quicker for you, I'm fine with closing the PR and leave it to you

Please close this PR and file a bug instead. I'll take care of updating basicHTML first then land CSS variables later on, thanks.

jiayihu commented 6 years ago

You do that only if you don't read what I write 😜

😅

The source of truth is the esm/* folder

Ah good to know, it sounded in fact very strange to me having to triplicate code.

Nope, not at all. It's basicHTML 'cause jsdom doesn't support (didn't?) Custom Elements and it's too strict with the standard + slower and bloated for my needs.

That makes sense because jsdom has setProperty method, it just apparently don't do nothing with custom properties according to linked issue. This comment deceived me into thinking jsdom is used.

Please close this PR and file a bug instead. I'll take care of updating basicHTML first then land CSS variables later on, thanks.

👍