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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSS Variables (Custom Properties) 馃拝 #612

Merged
merged 1 commit into from Mar 8, 2018

Conversation

frenzzy
Copy link
Contributor

@frenzzy frenzzy commented Feb 21, 2018

Usage:

<main style={{ '--foo': 'red' }}>
  <h1 style={{ color: 'var(--foo)' }}>Hello</h1>
  <button style={{ backgroundColor: 'var(--foo)' }}>+</button>
</main>

Refs:

@jorgebucaran jorgebucaran added the enhancement New feature or request label Feb 21, 2018
@frenzzy
Copy link
Contributor Author

frenzzy commented Feb 21, 2018

FYI, you also can use cssText as a workaround for current version of hyperapp:

<main style={{ cssText: '--foo:red' }}>
  <h1 style={{ color: 'var(--foo)' }}>Hello</h1>
  <button style={{ backgroundColor: 'var(--foo)' }}>+</button>
</main>

Demo: https://codepen.io/frenzzy/pen/RQygyL?editors=0010

@jorgebucaran
Copy link
Owner

Thanks, @frenzzy. Amazing work. It's not as much code as I actually imagined, but I'll not merge this before I can do some research of my own. Exploring alternatives like using cssText to implement inline styles, or maybe even something more wacko like Picostyle.

test/dom.test.js Outdated
tree: h("div", { style: { color: "blue", float: "left" } }),
tree: h("div", {
style: { color: "blue", float: "left", "--foo": "blue" }
}),
html: `<div style="color: blue; float: left;"></div>`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like jsdom does not support custom properties yet =/

test('example', () => {
  const div = document.createElement('div')
  div.style.setProperty('--foo', 'red')
  document.body.appendChild(div)

  console.log(document.body.innerHTML) // => '<div></div>'
  console.log(div.style.getPropertyValue('--foo')) // => undefined

  expect(document.body.innerHTML).toBe('<div style="--foo:red"></div>') // => fail
  expect(div.style.getPropertyValue('--foo')).toBe('red') // => fail
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Google Search crawler is based on Chrome 41, which does NOT support CSS Custom Properties: https://twitter.com/ebidel/status/968989651888295936

@codecov
Copy link

codecov bot commented Mar 8, 2018

Codecov Report

Merging #612 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #612   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         152    155    +3     
  Branches       48     49    +1     
=====================================
+ Hits          152    155    +3
Impacted Files Coverage 螖
src/index.js 100% <100%> (酶) 猬嗭笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 1e47432...11d65a5. Read the comment docs.

@jorgebucaran
Copy link
Owner

@frenzzy What's the total byte increase?

@frenzzy
Copy link
Contributor Author

frenzzy commented Mar 8, 2018

file master branch diff
hyperapp.js 9106 9235 +129
hyperapp.min.js 3465 3507 +42
hyperapp.min.js.gz 1545 1568 +23

@jorgebucaran
Copy link
Owner

@frenzzy Sounds like a reasonable compromise. Is there anything else to add? Does this cover everything?

What about --mainColor or $mainColor instead of --main-color? Like:

<div style={{
  $cssVariable: "red"
}} />

Probably a lot more code, but it would be cool haha.

/cc @lukejacksonn @zaceno @SkaterDad @okwolf

@frenzzy
Copy link
Contributor Author

frenzzy commented Mar 8, 2018

According to custom properties specification the only requirement is any custom property name must starts with two dashes.

So, what notation to use (--mainColor or --main-color) is up to you.

Symbol $ is more about preprocessors like Sass, Less an so on, but not about native CSS.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Mar 8, 2018

@frenzzy

So, what notation to use (--mainColor or --main-color) is up to you.

You are right.

Symbol $ is more about preprocessors like Sass, Less an so on, but not about native CSS.

Right, but if we (just an idea) treated names starting with a $ as a CSS variable it would be a lot easier to use/type in the style object.

{ $cssVariable: "red" }

...instead of:

{ "--cssVariable": "red" }

But yeah, I'd probably be crucified if we introduced this convenient oddball.

@lukejacksonn
Copy link
Contributor

For once, I am not totally against breaking the standard here as you point out $var is way way better than "--var" but it is probably still a bad idea 馃槄

@frenzzy
Copy link
Contributor Author

frenzzy commented Mar 8, 2018

How about this?

const brandColor = '--brand-color'
const $brandColor = `var(${brandColor})` 

const view = () => (
  <main style={{ [brandColor]: 'red' }}>
    <h1 style={{ color: $brandColor }}>Hello</h1>
    <button style={{ backgroundColor: $brandColor }}>+</button>
  </main>
)

@jorgebucaran jorgebucaran changed the title CSS Variables (Custom Properties) CSS Variables (Custom Properties) 馃拝 Mar 8, 2018
@jorgebucaran
Copy link
Owner

jorgebucaran commented Mar 8, 2018

@frenzzy What about rewriting the code like this?

for (var i in clone(oldValue, value)) {
  if (i[0] === "-") {
    element[name].setProperty(i, value)
  } else {
    element[name][i] = value == null || value[i] == null ? "" : value[i]
  }
}

Or what about using only setProperty for both cases?

for (var i in clone(oldValue, value)) {
  element[name].setProperty(i, value == null || value[i] == null ? "" : value[i])
}

@frenzzy frenzzy force-pushed the css-properties branch 2 times, most recently from f3c71b3 to 7cacaea Compare March 8, 2018 14:16
@jorgebucaran
Copy link
Owner

Test #ff0000 #ff00ff #ffff00 #ffffff

@jorgebucaran
Copy link
Owner

jorgebucaran commented Mar 8, 2018

@frenzzy frenzzy force-pushed the css-properties branch 3 times, most recently from 5a5d83b to 85de826 Compare March 8, 2018 14:49
@jorgebucaran jorgebucaran merged commit d91e466 into jorgebucaran:master Mar 8, 2018
@frenzzy frenzzy deleted the css-properties branch March 9, 2018 02:33
@SteveALee
Copy link

So, what notation to use (--mainColor or --main-color) is up to you.

But I suggest stick to the current convention of snale case in CSS and camel case in JS

PS I only just found out you can set CSS vars easily in JS. no need to mess about manipulating stylesheets in JS.

@jorgebucaran
Copy link
Owner

@SteveALee We went with the --no-surprise version of CSS variables! 馃槈

<div
  style={{
    "--color": "red",
    color: "var(--color)"
  }}
>
  OK
</div>

@SteveALee
Copy link

Totally Agree. Just making a note for users of HA :)

@jorgebucaran jorgebucaran self-requested a review March 9, 2018 12:08
Copy link
Owner

@jorgebucaran jorgebucaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants