[wip] Migrate to React #91
Conversation
| // handling for, like undo/redo etc. | ||
| const zKeyCode = 90; | ||
| if (e.metaKey && e.keyCode === zKeyCode) { | ||
| this.setState({ undone: true }); |
There was a problem hiding this comment.
makes it so that the linter doesn't throw because we're not using this in a class method. i could just extract the method but it seemed like it made more sense for an example to have it on the class
example/client.js
Outdated
|
|
||
| const app = tree(<Article items={items} onUpdate={onUpdate} getCustomKeyDown={getCustomKeyDown} />); | ||
| return () => {}; |
There was a problem hiding this comment.
Should not return an empty function here, we only want to return a handler if this is an event we want to handle.
There was a problem hiding this comment.
ok; we have to return something for the linter, so i'll return undefined
|
|
||
| ```sh | ||
| npm install article-json-to-contenteditable --save | ||
| ``` | ||
| ## Example |
There was a problem hiding this comment.
If you move the example/client.js file to the root and name it example.js I think it will be included in the readme automatically. Might be nice, wdyt?
| return; | ||
| } | ||
| // We need to update the figure without touching the iframe | ||
| // to avoid reloading it for every update. | ||
| const oldCaption = oldFigure.querySelector('figcaption'); | ||
| const newCaption = newFigure.querySelector('figcaption'); | ||
| const oldCaption = oldFigure.getElementsByTagName('figcaption')[0]; |
There was a problem hiding this comment.
Why are these changed from querySelector to getElementsByTagName?
There was a problem hiding this comment.
flow can apparently understand getElementsByTagName better
There was a problem hiding this comment.
lib/patch-dom.js
Outdated
| const oldCaption = oldFigure.querySelector('figcaption'); | ||
| const newCaption = newFigure.querySelector('figcaption'); | ||
| const oldCaption = oldFigure.getElementsByTagName('figcaption')[0]; | ||
| const newCaption = newFigure.getElementsByTagName('figcaption'); |
There was a problem hiding this comment.
This should be newFigure.getElementsByTagName('figcaption')[0]
| @@ -63,8 +71,8 @@ export default ({oldArticleElm, newArticleElm}) => { | |||
| case UPDATE: | |||
| if (isIframeEmbed(prev)) { | |||
| queue.update.push(() => updateIframeEmbed({ | |||
| oldFigure: oldArticleElm.childNodes[pos], | |||
| newFigure: next | |||
| oldFigure: oldArticleElm.children[pos], | |||
There was a problem hiding this comment.
Why use .children instead of .childNodes?
There was a problem hiding this comment.
it doesn't work with Flow for some reason
There was a problem hiding this comment.
Hm, ok strange. Is flow not complaining about the other places we're using childNodes in here?
It's probably fine to change this here, since we actually are looking for an element and not a node. Not sure our tests are covering this a 100% though so we should just make sure manually that this is working as expected.
lib/index.js
Outdated
| @@ -116,7 +117,8 @@ const setup = ({ customTextFormattings, parseFigureProps, customCaption }: Setup | |||
| // Check props first without rendering, | |||
| // no need to go through with rendering if we know nothing has changed. | |||
| const propsHasUpdated = this.props.selections !== nextProps.selections || | |||
| this.props.contentEditable !== nextProps.contentEditable; | |||
| this.props.contentEditable !== nextProps.contentEditable || | |||
| !deepEqual(this.props.items, nextProps.items, { strict: true }); | |||
There was a problem hiding this comment.
@ryanscottaudio Hey, did this make a performance difference? Realizing now it might do that in the example, but where we use this in our editor we're using immutable data, so just checking this.props.items !== nextProps.items used to be enough before at least.
TODO: