Fix React forwardRef warnings for TooltipAnchors#54492
Merged
Merged
Conversation
kevin940726
added
[Type] Bug
kevin940726
requested review from
aaronrobertshaw,
brookewp,
ciampo and
glendaviesnz
|
Size Change: -146 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Sep 15, 2023
ciampo
left a comment
ciampo
left a comment
Contributor
There was a problem hiding this comment.
Thank you for working on this so quickly!
These changes look ok to me, I don't think they could any negative impact.
Could you add a CHANGELOG entry too, please?
Comment on lines
+11
to
+14
| * @param {IconProps} props icon is the SVG component to render | ||
| * size is a number specifiying the icon size in pixels | ||
| * Other props will be passed to wrapped SVG component | ||
| * @param {import('react').Ref<SVGSVGElement>} ref The forwarded ref to the SVG element. |
Contributor
There was a problem hiding this comment.
Since Icon could be any JSX element, it's probably safer to go with any (we do this in almost every components in the package)
Also, we usually use the ForwardedRef type
Suggested change
| * @param {IconProps} props icon is the SVG component to render | |
| * size is a number specifiying the icon size in pixels | |
| * Other props will be passed to wrapped SVG component | |
| * @param {import('react').Ref<SVGSVGElement>} ref The forwarded ref to the SVG element. | |
| * @param {IconProps} props icon is the SVG component to render | |
| * size is a number specifiying the icon size in pixels | |
| * Other props will be passed to wrapped SVG component | |
| * @param {import('react').ForwardedRef<any>} ref The forwarded ref to the SVG element. |
Member
Author
There was a problem hiding this comment.
I use the SVGSVGElement type because the description of the icon prop says that it should render an SVG component. Do you think if it matters here? I've changed that to HTMLElement for now as I think it's probably better than any? 😆
Comment on lines
+87
to
+89
| * @param {SVGProps} props isPressed indicates whether the SVG should appear as pressed. | ||
| * Other props will be passed through to svg component. | ||
| * @param {import('react').Ref<SVGSVGElement>} ref The forwarded ref to the SVG element. |
Contributor
There was a problem hiding this comment.
Same re. ForwardedRef
Suggested change
| * @param {SVGProps} props isPressed indicates whether the SVG should appear as pressed. | |
| * Other props will be passed through to svg component. | |
| * @param {import('react').Ref<SVGSVGElement>} ref The forwarded ref to the SVG element. | |
| * @param {SVGProps} props isPressed indicates whether the SVG should appear as pressed. | |
| * Other props will be passed through to svg component. | |
| * @param {import('react').ForwardedRef<SVGSVGElement>} ref The forwarded ref to the SVG element. |
aaronrobertshaw
approved these changes
Sep 18, 2023
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the quick fix on this @kevin940726 🚀
It tests well for me.
After a changelog is added and the rest of @ciampo's suggested tweaks are made, this is good to go in my books.
ciampo
approved these changes
Sep 18, 2023
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
3 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?
A bug introduced in #54450. This PR fixes the warning as originally reported in #53835 (comment).
Why?
Even though it's only a warning, it prevents the ref from being passed to the underlying DOM element, which might cause some unexpected behaviors.
How?
<TooltipAnchor>implicitly requires the rendered child to be wrapped byforwardRefso that it can pass its own refs down to the element. This is briefly mentioned [here] in the docs.This PR adds
forwardRefto both the<Icon>and the<SVG>component.Testing Instructions
http://localhost:8888/wp-admin/site-editor.php?path=%2Fpatterns&categoryType=pattern&categoryId=all-patternsforwardRefwhen there are tooltips on the page.Testing Instructions for Keyboard
Same as above, it's a technical change that doesn't affect the UI.
Screenshots or screencast
N/A