#26 - Fix style prop #27
Conversation
| @@ -55,6 +55,8 @@ export default function (CustomElement, opts) { | |||
|
|
|||
| if (name.indexOf('on') === 0) { | |||
| syncEvent(node, name.substring(2), props[name]); | |||
| } else if (name === 'style') { | |||
| node.setAttribute('style', props[name]); | |||
There was a problem hiding this comment.
Interesting, so React transforms style={{ display: 'block' }} to CSS?
There was a problem hiding this comment.
Do we care about ref and className, etc?
There was a problem hiding this comment.
@bradleyayers it seems so.
@stevemao className should work since we're passing it as a property, right? I'll write a test for that. ref should be a separate issue, I think. Can you raise it please?
There was a problem hiding this comment.
@stevemao I added a test for className to ensure that works.
@bradleyayers it seems React actually doesn't do that. I was getting a false positive. Instead I decided to just pass-through style but we needed to make sure it was being passed as the attrs argument to React.createElement() for the test to pass.
There was a problem hiding this comment.
@treshugart so what does React actually do with a style object — how is that applied to the DOM.
There was a problem hiding this comment.
@bradleyayers Discussed IRL, but for posterity:
- React enforces you pass a style object that maps to a CSSStyleDeclaration.
- We use
willReceiveProps()to set properties on the element. In this lifecycle callback,styleis passed in as an object. - We must pass the
styleprop toReact.createElement()asattributesin order for React to apply its special behaviour to it (i.e. converting to a string and numbers havepxappended to them.
|
LGTM |
… false positive. Reordered tests so that built-in React props come first. #26
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@treshugart Yup sorry
What about {this.props}. Can we just pass all props down without explicitly listing everything?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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);There was a problem hiding this comment.
That'll set attributes instead of properties.
|
lgtm |
Fixes #26.