Move escape key handling to modal component #2013
Conversation
nextstrain-bot
temporarily deployed
to
auspice-victorlin-move--9ade09
| useEffect(() => { | ||
| function handleEscapeKey(event) { | ||
| if (event.key==="Escape" && selectedNode) { | ||
| clearSelectedNode(selectedNode); | ||
| } | ||
| } | ||
| document.addEventListener('keyup', handleEscapeKey); | ||
| return function cleanup() { | ||
| document.removeEventListener('keyup', handleEscapeKey); | ||
| }; | ||
| }, [selectedNode, clearSelectedNode]); | ||
|
|
There was a problem hiding this comment.
One salient difference is that this will add/remove listeners every time a modal appears/disappears as opposed to a single listener set up when the tree panel mounts. In reality this won't matter because opening a modal is a one-off action, but it's something to be aware of when designing listeners.
An improvement would be to return early to avoid adding an unnecessary listener when we close the modal:
useEffect(() => {
if (!selectedNode) return
...There was a problem hiding this comment.
Thanks for noting the difference. There is a way to retain the existing behavior using a ref, which I've incorporated into the main commit (c136e25). I added another commit to switch to the plain useEffect pattern with your proposed improvement above (eae3fba), which we can drop if you think the existing behavior is better.
There was a problem hiding this comment.
I need to dive back into the world of hooks one day, although I wasn't very enamored with them. Perhaps the rest of the group feel differently! Is adding an additional useRef & useEffect the recommended way? I find it somewhat convoluted to read, although I can figure out how it works. Up to you whether this is your preferred method vs. a single but slightly more costly useEffect - either works for me.
There was a problem hiding this comment.
I used useRef & useEffect in c136e25 with the goal of retaining the existing behavior (adding the event listener once on component mount). I don't think it's the recommended way, and also find it convoluted to read.
The single useEffect in eae3fba aligns more with examples in docs, suggesting that it’s the recommended way. It's not necessarily more costly (depends on how we define "cost"), which I tried to reflect in the commit message.
|
P.S. "The Effect Hook is a direct replacement for componentDidMount and componentWillUnmount" just isn't true. Perhaps |
The new name better reflects existing usage.
This co-locates handling of the escape key within the component that it is used for, where clearSelectedNode is already used as an onClick callback to close the modal. useEffect is the Hooks replacement¹ for componentDidMount and componentWillUnmount. selectedNodeRef has been added to replicate the behavior of adding the event listener once on component render. Without the ref, selectedNode would need to be set as a dependency, which would add/remove the event listener on every change of selectedNode. ¹ https://legacy.reactjs.org/docs/hooks-effect.html#effects-with-cleanup
This adds/removes the event listener on every change of selectedNode, but has the benefit of not running the event listener on every keyup even when no node is selected.
victorlin
force-pushed
the
victorlin/move-esc-modal-code
branch
from
fbae400 to
eae3fba
Compare
Reworded in c136e25. |
Description of proposed changes
This co-locates handling of the escape key within the component that it is used for, where clearSelectedNode is already used as an onClick callback to close the modal.
Related issue(s)
Thought of this while investigating #2011. While it doesn't fix the bug, it helps answer the first question that came to mind: "how different are the esc and click handling code paths?"
Checklist
If making user-facing changes, add a message in CHANGELOG.md summarizing the changes in this PRno functional changes