Add otherProps util and DangerouslySetHTML component #242
Conversation
|
Not sure why this is failing on travis... |
There was a problem hiding this comment.
@jbalsas I added a few nitpicks, but all in all, this seems good. Thanks @bryceosterhaus!
| */ | ||
| class DangerouslySetHTML extends Component { | ||
| /** | ||
| * @return {Component} |
There was a problem hiding this comment.
Indentation looks a little funny here.
packages/metal-jsx/src/otherProps.js
Outdated
|
|
||
| const retObj = object.mixin({}, component.props); | ||
|
|
||
| removeKeys.forEach( |
There was a problem hiding this comment.
I'm not sure if this really needs to change, but I can see most of the core library mostly prefers the C-style for loop over forEach. I'm guessing for performance? (The overhead of a function invocation for n items).
| }); | ||
|
|
||
| it('should ignore key and ref', function() { | ||
| class TestComponent extends JSXComponent { |
There was a problem hiding this comment.
I wonder if we can maybe start writing new tests completely in ES6? I mean, we are using class keywords already so...
|
@bryceosterhaus Oh, I just realized something. Maybe this could be a class method on class MyComponent extends JSXComponent {
render() {
return <div ...this.otherProps() class="content" />;
}
} |
|
@mthadley, not a bad idea. Seems a little strange at first glance, but probably just because I am used to it being a separate function. Also, for es6 in the unit tests, I decided to just follow the same patterns we had in our other tests |
|
@bryceosterhaus I figured you were, which is fine. But maybe going forward we can start pushing for more idiomatic ES6. I'll defer to you and @jbalsas on this. |
|
Hey guys, thanks for working on this! If @bryceosterhaus, can you make that change and let me know? Regarding the tests, I'd love to start writing them in ES6 moving forward, but I wouldn't hold this PR for that, so no need to rewrite them right now. Thanks! |
|
LGTM! @mthadley, one final check and I'll merge 😉 |
|
🎉 @jbalsas LGTM 🎉 |
|
LGTM |
|
Could you add a test case for using |
|
@eduardolundgren, added additional test case here |
|
LGTM |
From discussion on #239
Fixes #127
Fixes #205