-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Remove unused components from ui/
#54573
Changes from all commits
9658f1d
3f9a205
143d1a6
bdf2226
2d39b03
7d287e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
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 you please elaborate on this change?
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.
Yes, thank you for asking! I meant to leave a comment here when I opened the PR.
There was a conflict between the
onBlur
added to the Tooltip anchor and the functionuseCopyToClipboard
in theColorPicker
component:gutenberg/packages/components/src/color-picker/color-copy-button.tsx
Line 21 in 70f787b
In this function, the
focus
is reset, resulting in the tooltip hiding on copy button click before displaying the 'copied' text:gutenberg/packages/compose/src/hooks/use-copy-to-clipboard/index.js
Lines 52 to 59 in 23bb930
Originally, we added the
onBlur
to the tooltip to replicate the legacy behavior where the tooltip is hidden when leaving the document. This was discovered through failing tests; but now, the tests pass without it. The utility function was added after, so it's likely the failure was related to leaky tests instead.I discussed removing this or adding it as a prop with @ciampo, and we decided to remove it for now. And if we find a use-case where it makes sense to have it, we could add it as an optional prop.
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.
Makes sense to me, thanks 👍
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.
We may need to consider adding a prop to customize this behaviour, since this change is related to a regression #57206