Update example, tests and dependecies for react 16 support #43
Conversation
| import PropTypes from 'prop-types' | ||
| import DOM from 'react-dom-factories' |
There was a problem hiding this comment.
I think we can get rid of this. This was preferable solution in CoffeeScript, now it's obsolete. Can we use JSX everywhere instead?
| @@ -28,8 +29,11 @@ describe('Shortcuts component', () => { | |||
| global.CustomEvent = window.CustomEvent | |||
| simulant = require('simulant') | |||
| ReactDOM = require('react-dom') | |||
| DOM = require('react-dom-factories') | |||
There was a problem hiding this comment.
Again, let's use JSX instead.
| @@ -116,18 +120,18 @@ describe('Shortcuts component', () => { | |||
| }) | |||
|
|
|||
| it('should have children', () => { | |||
| const props = _.assign({}, baseProps, { children: React.DOM.div() }) | |||
| const props = _.assign({}, baseProps, { children: DOM.div() }) | |||
There was a problem hiding this comment.
We can remove lodash and use Object.assign or { ...baseProps }.
| baseContext.shortcuts.setKeymap(editedKeymap) | ||
|
|
||
| simulant.fire(node, 'keydown', { keyCode: space }) | ||
| // sometimes node loose focus, so I added this timeout to fix it | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Is this timeout really necessary? Why did it work before without it?
There was a problem hiding this comment.
It's a good question. Currently I don't have time to deep debugging. I will back to this PR a bit later and will provide more info (or solution without monkey-patching which must be). If you do not do this faster than me sure :)
|
I'm interested in getting rid of the yarn warnings about the peerDependency on react when using react v16. Is this PR still in the pipes? |
|
@petrbrzek @dimapaloskin can we move this forward in order to officially support react 16? |
|
The problem with this PR is that it is trying to do too much. |
Hello.
I updated dependencies, example and tests for supporting new react 16.
What do you think about it?