Remove unused components from ui/
#54573
Merged
Conversation
This file contains 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
brookewp
added
[Type] Enhancement
|
Size Change: -9.75 kB (-1%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 7d287e4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6266152210
|
tyxla
approved these changes
Sep 19, 2023
| @@ -73,7 +73,6 @@ function Tooltip( props: TooltipProps ) { | |||
| return ( | |||
| <> | |||
| <Ariakit.TooltipAnchor | |||
| onBlur={ tooltipStore.hide } | |||
There was a problem hiding this comment.
Could you please elaborate on this change?
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.
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" |
There was a problem hiding this comment.
Is this change necessary and/or related to the tooltip swap?
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.
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
brookewp
force-pushed
the
remove/ui-tooltip
branch
from
70f787b
to
d21d913
Compare
ciampo
approved these changes
Sep 21, 2023
tyxla
approved these changes
Sep 21, 2023
brookewp
force-pushed
the
remove/ui-tooltip
branch
from
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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