Skip to content
This repository was archived by the owner on Sep 7, 2022. It is now read-only.

#26 - Fix style prop #27

Merged
merged 5 commits into from
Jul 12, 2016
Merged

#26 - Fix style prop #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f211c3d
test(props): Write test to ensure setting "style" does not work.
treshugart Jul 11, 2016
2dc7273
test(props): Add another test to ensure style is set properly using a…
treshugart Jul 11, 2016
aadcc16
fix(props): Ensure style is set correctly for both an object and a st…
treshugart Jul 11, 2016
d66f304
fix(props): Simplified fix and corrected test code that was getting a…
treshugart Jul 11, 2016
709673e
test(rest/spread): Add a test to ensure rest/spread properties work.
treshugart Jul 11, 2016
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
5 changes: 3 additions & 2 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export default function (CustomElement, opts) {
static get propTypes() {
return {
children: React.PropTypes.any,
style: React.PropTypes.any,
};
}
componentDidMount() {
Expand All @@ -49,7 +50,7 @@ export default function (CustomElement, opts) {
componentWillReceiveProps(props) {
const node = ReactDOM.findDOMNode(this);
Object.keys(props).forEach(name => {
if (name === 'children') {
if (name === 'children' || name === 'style') {
return;
}

Expand All @@ -61,7 +62,7 @@ export default function (CustomElement, opts) {
});
}
render() {
return React.createElement(tagName, null, this.props.children);
return React.createElement(tagName, { style: this.props.style }, this.props.children);
Copy link
Member

Choose a reason for hiding this comment

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

What about {...this.props}?

Copy link
Member

Choose a reason for hiding this comment

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

Would be transpiled to a call that contains Array.from. One of our mobile browsers we need to support doesnt know that yet if I remember correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joscha that's for objects.

@stevemao It's still in TC39 stage 2. I'll add a test since it's common practice in React, but I'll note that there's no special behaviour necessary for this so we're essentially testing Babel here.

Copy link
Member

Choose a reason for hiding this comment

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

@treshugart Yup sorry
What about {this.props}. Can we just pass all props down without explicitly listing everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you can just do <Element {...this.props} /> and everything will get set through our willReceiveProps() callback (which is what we use to ensure props are set as properties on the web component).

Copy link
Member

Choose a reason for hiding this comment

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

I mean here instead of

return React.createElement(tagName, { style: this.props.style }, this.props.children);

Is there any downside to just simply

return React.createElement(tagName, { this.props }, this.props.children);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'll set attributes instead of properties.

}
};
}
62 changes: 45 additions & 17 deletions test/unit/props.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,41 +3,69 @@ import React from 'react';
import ReactDOM from 'react-dom';

let x = 0;
function createComponent(name, done) {
function createComponentWithOpts(opts) {
return reactify(document.registerElement(`x-props-${x++}`, {
prototype: Object.create(HTMLElement.prototype, {
[name]: {
get() {
return 'test';
},
set(value) {
if (done) {
done(this, value);
}
},
},
}),
prototype: Object.create(HTMLElement.prototype, opts),
}), { React, ReactDOM });
}
function createComponentWithProp(name, done) {
return createComponentWithOpts({
[name]: {
get() {
return 'test';
},
set(value) {
if (done) {
done(this, value);
}
},
},
});
}

describe('props', () => {
it('should set style (object)', () => {
const Comp = createComponentWithOpts({});
ReactDOM.render(<Comp style={{ backgroundColor: 'black', width: 1 }} />, window.fixture);
const elem = window.fixture.firstChild;
expect(elem.style.backgroundColor).to.equal('black');
expect(elem.style.width).to.equal('1px');
});

it('should set className', () => {
const Comp = createComponentWithOpts({});
ReactDOM.render(<Comp className="test" />, window.fixture);
const elem = window.fixture.firstChild;
expect(elem.getAttribute('class')).to.equal('test');
});

it('should not set children', () => {
const Comp = createComponent('children', () => { throw new Error('set children'); });
const Comp = createComponentWithProp('children', () => { throw new Error('set children'); });
ReactDOM.render(<Comp><div /></Comp>, window.fixture);
});

it('should not set events', () => {
const Comp = createComponent('oncustomevent', () => { throw new Error('set oncustomevent'); });
const Comp = createComponentWithProp('oncustomevent', () => { throw new Error('set oncustomevent'); });
ReactDOM.render(<Comp oncustomevent="test" />, window.fixture);
});

it('should not set attributes', () => {
const Comp = createComponent('test', elem => expect(elem.hasAttribute('test')).to.equal(false));
const Comp = createComponentWithProp('test', elem => expect(elem.hasAttribute('test')).to.equal(false));
ReactDOM.render(<Comp test="test" />, window.fixture);
});

it('should set properties for anything else', () => {
const Comp = createComponent('test', (elem, value) => expect(value).to.equal('test'));
const Comp = createComponentWithProp('test', (elem, value) => expect(value).to.equal('test'));
ReactDOM.render(<Comp test="test" />, window.fixture);
});

it('should work with rest/spread properties', () => {
const Comp = createComponentWithProp('test');
const rest = { style: { color: 'white' }, test: 'test' };
ReactDOM.render(<Comp className="test" {...rest} />, window.fixture);
const elem = window.fixture.firstChild;
expect(elem.getAttribute('class')).to.equal('test');
expect(elem.style.color).to.equal('white');
expect(elem.test).to.equal('test');
});
});