-
Notifications
You must be signed in to change notification settings - Fork 1
fix(Modal): Use Transition to simplify animation; add close button; convert to hooks #24
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24 +/- ##
==========================================
+ Coverage 71.91% 74.84% +2.92%
==========================================
Files 12 12
Lines 146 163 +17
Branches 19 24 +5
==========================================
+ Hits 105 122 +17
Misses 31 31
Partials 10 10
Continue to review full report at Codecov.
|
src/Modal/Modal.js
Outdated
useEffect(() => { | ||
document.addEventListener('keydown', handleKeyDown); | ||
return () => document.removeEventListener('keydown', handleKeyDown); | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth conditionally firing with [isOpen, props.closeOnEscape]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just gave that a try. Presented weird behavior that closes both modals and reopens the second when pressing escape
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. When testing locally, I noticed a non-blocking, existing error regarding the .mdx
file not providing keys to the mapped <p>
in the console.
No description provided.