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.

Sign up for GitHub

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

Jump to bottom

#666 support css variables #827

Closed
wants to merge 10 commits into from
Closed

#666 support css variables #827

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0f08806
Merge branch 'master' of https://github.com/developit/preact
May 19, 2017
eec3951
Merge branch 'master' of https://github.com/developit/preact
Aug 22, 2017
8a8e685
#666 - Add support for CSS Custom Properties
Aug 22, 2017
7273c81
- optimize for size and perf in cases when a css property starts
Aug 23, 2017
b98bc18
- use setProperty instead of removeProperty to optimize for gzip
Aug 24, 2017
7cbcc0d
Merge branch 'master' into 666-support-css-variables
developit Aug 24, 2017
db44005
Merge branch 'master' into 666-support-css-variables
developit May 22, 2018
79ca673
fix tests
developit May 22, 2018
d9bca90
Merge branch 'master' into 666-support-css-variables
tkh44 May 29, 2018
b2c742e
Merge branch 'master' into 666-support-css-variables
developit Aug 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/dom/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,21 @@ export function setAccessor(node, name, old, value, isSvg) {
}
if (value && typeof value==='object') {
if (typeof old!=='string') {
for (let i in old) if (!(i in value)) node.style[i] = '';
for (let i in old) if (!(i in value)) {
Copy link
Member

Choose a reason for hiding this comment

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

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]);
  }
}

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@PepsRyuu aye, saw this after 🙈 /delete

if (i.indexOf('--') === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

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'.

Copy link
Member

@developit developit Aug 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Damn GitHub collapsing comments. Missed this

node.style.removeProperty(i);
} else {
node.style[i] = '';
}
}
}
for (let i in value) {
node.style[i] = typeof value[i]==='number' && IS_NON_DIMENSIONAL.test(i)===false ? (value[i]+'px') : value[i];
if (i.indexOf('--') === 0) {
node.style.setProperty(i, value[i]);
}
else {
node.style[i] = typeof value[i]==='number' && IS_NON_DIMENSIONAL.test(i)===false ? (value[i]+'px') : value[i];
}
}
}
}
Expand Down
13 changes: 8 additions & 5 deletions test/browser/render.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,10 +239,11 @@ describe('render()', () => {
});

it('should apply style as String', () => {
render(<div style="top:5px; position:relative;" />, scratch);
render(<div style="top:5px; position:relative; --var-font-size: 12px; font-size: var(--var-font-size);" />, scratch);
expect(scratch.childNodes[0]).to.have.deep.property('style.cssText')
.that.matches(/top\s*:\s*5px\s*/)
.and.matches(/position\s*:\s*relative\s*/);
.and.matches(/position\s*:\s*relative\s*/)
.and.matches(/--var-font-size\s*:\s*12px\s*/);
});

it('should only register on* functions as handlers', () => {
Expand Down Expand Up @@ -356,7 +357,8 @@ describe('render()', () => {
'background-size': 'cover',
padding: 5,
top: 100,
left: '100%'
left: '100%',
'--var-font-size': '12px'
}}>
test
</div>
Expand All @@ -370,12 +372,13 @@ describe('render()', () => {
expect(style).to.have.property('padding', '5px');
expect(style).to.have.property('top', '100px');
expect(style).to.have.property('left', '100%');
expect(style.getPropertyValue('--var-font-size')).to.equal('12px');

root = render((
<div style={{ color: 'rgb(0, 255, 255)' }}>test</div>
<div style={{ color: 'rgb(0, 255, 255)', '--var-font-size': '24px' }}>test</div>
), scratch, root);

expect(root).to.have.deep.property('style.cssText').that.equals('color: rgb(0, 255, 255);');
expect(root).to.have.deep.property('style.cssText').that.equals('color: rgb(0, 255, 255); --var-font-size:24px;');

root = render((
<div style="display: inline;">test</div>
Expand Down