#16, #17, #19 and #20 - Remove portals, use native element constructors, support custom events and upgrade to use semantic-release / commitizen. #18
Conversation
treshugart
changed the title
… of the new component wrapper. Also fixed running of tests and displayName stuff.
| @@ -1,6 +1,6 @@ | |||
| The MIT License (MIT) | |||
|
|
|||
| Copyright (c) {year} {author} | |||
| Copyright (c) Trey Shugart <treshugart@gmail.com> 2016 | |||
There was a problem hiding this comment.
This is flipped now, probably won't matter though :)
There was a problem hiding this comment.
Interesting. That's a bit odd. I didn't touch the LICENSE template or file manually.
|
LGTM, however I think it would be beneficial to read the config files (webpack, karma, saucelabs) from skatejs-build like we do in named-slots if possible. |
|
Re the config files, I agree and that's been done. Originally I wanted to do this but it got lost in the midst of doing other fixes. Waiting for the build to re-release now. |
| @@ -0,0 +1,3 @@ | |||
| describe('skatejs-react-integration', () => { | |||
|
|
|||
There was a problem hiding this comment.
Thanks, wrote it myself ;)
|
LGTM 👍 |
…ng older browsers.
… polyfill and triggering built-
…to case-sensitivity.
|
|
||
| // Listen for custom element changes and cancel them to make a stateless component | ||
| this._realCustomElement.addEventListener('property-change', function (e) { | ||
| if (props.handlePropertyChange) { |
There was a problem hiding this comment.
There was some behaviour before where if you passed a handlePropertyChange prop, it'd cancel all state change events and give that data back to React. Is there an equivalent now?
There was a problem hiding this comment.
I ended up keeping it and writing tests for it. In doing so, I realised all it's really doing is automating the binding to the prop-change event and calling e.preventDefault(). Now that we support custom events, all you have to do is simply bind to the event and call e.preventDefault() yourself. Removing it prevents us from having to maintain and test this behaviour. We'd also want to allow customisation of the event name and prop handler name. Now all you need to do is:
<Component onPropChange={e => e.preventDefault()} />...if propchange was emitted when a property changes.
treshugart
changed the title
|
LGTM. This should also fix #11 |
|
I'll update #11 to remind us to write a test for it. |
Implements #16, #17, #19 and #20. Bulk of the review
src/index.js. It was simpler to do all of these at once since it was a major overhaul to the only file in this repo. The rest of the files were changed / added / removed from re-initialising from the updated build.This PR also removes the old tests and adds new ones in their place as the old tests were tested against Skate and the portals method. Since we've generalised the wrapper and removed portal support, these tests were no longer relevant.