Close channel/tx detail on escape press #455
Conversation
valentinewallace
force-pushed
the
close-widgets-on-esc
branch
from
2a78456 to
8e69e22
Compare
valentinewallace
force-pushed
the
close-widgets-on-esc
branch
from
8e69e22 to
becd6af
Compare
src/component/modal.js
Outdated
| super(props); | ||
| this.handleKeyDown = this.handleKeyDown.bind(this); | ||
| } | ||
| componentDidMount() { |
There was a problem hiding this comment.
Missing empty line between the two functions.
src/component/modal.js
Outdated
| constructor(props) { | ||
| props.title = props.title || ''; | ||
| super(props); | ||
| this.handleKeyDown = this.handleKeyDown.bind(this); |
There was a problem hiding this comment.
Not a fan of the bind style. We should use arrow functions in the event handlers themselves to stay consistent with our current coding style.
src/component/modal.js
Outdated
| this.handleKeyDown = this.handleKeyDown.bind(this); | ||
| } | ||
| componentDidMount() { | ||
| document.addEventListener('keydown', this.handleKeyDown); |
There was a problem hiding this comment.
We should check if document exist as this will crash on mobile.
Also please use e => this.handleKeyDown(e) instead of bind.
src/component/modal.js
Outdated
|
|
||
| componentWillUnmount() { | ||
| document.removeEventListener('keydown', this.handleKeyDown); | ||
| } |
| </View> | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm wondering if we should pull this out into an Escapable component that the modal inherits from.
Check for (typeof document !== ‚undefined‘)
tanx
left a comment
tanx
left a comment
There was a problem hiding this comment.
Great! I made two minor changes. Please take a look. Otherwise ready to merge from my side.
|
|
||
| componentWillUnmount() { | ||
| if (typeof document !== 'undefined') { | ||
| document.removeEventListener('keydown', this.handleKeyDown); |
There was a problem hiding this comment.
@valentinewallace it turn out we needed to use the bind style after all otherwise the function pointer would not have been consistent and the event listener would not have been removed.
| } | ||
|
|
||
| componentDidMount() { | ||
| if (typeof document !== 'undefined') { |
There was a problem hiding this comment.
When checking for a global like document or window always use typeof otherwise you'll get this error in an environment that does not have them: ReferenceError: document is not defined
Closes #333