Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modal: add exit animation #65203
Modal: add exit animation #65203
Changes from all commits
aadbf93
d488176
5a87b55
d16d293
e1a0ed3
be87876
85668ad
352e057
4185a82
25d8538
11c0518
3686a86
d774215
bbb49a7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I removed the named imports in favour of using the
React
global namespace because IMO it makes much easier to distinguish between native event types (ie.KeyboardEvent
) and react synthetic event types (ie.React.KeyboardEvent
).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.
Could we add an explicit
import type * as React from 'react'
so that we don't use theReact
name without importing it? That also achieves the goal and is easier to understand. For example I don't understand where does theReact
namespace magically appear from 🙂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.
I came across two issues when doing that:
@typescript-eslint/consistent-type-imports
ESLint rule with the error "Type import "React" is used by decorator metadata.". ESLint wants to auto-fix this toimport * as React from 'react'
@typescript-eslint/no-restricted-imports
rule is broken, since ESLint would like us to import it from@wordpress/element
I don't think the auto-fix makes sense, since we only want to import the types — but at the same time, I don't understand the first error, and I'm not sure if it's safe to ignore.
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.
onRequestClose
in theory should be always defined, no need for theif
checkThere 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.
I extracted
handleAnimationEnd
to a variable outside of thestartAnimation
function, so that it can be used at the end of the promise "race" to remove that event listener. Happy to hear suggestions if folks have a more elegant way to do so