-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add Popover targetElementTag prop #2276
Conversation
Thanks for your interest in palantir/blueprint, @tgreen7! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
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.
@tgreen7 sweet, glad this was easy! please rename for consistency with existing prop.
* Component element of the popover target. | ||
* @default "div" | ||
*/ | ||
targetComponent?: string; |
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.
rename to targetElementTag
to match rootElementTag
, and update docs to match the existing prop.
* Component element of the popover target. | ||
* @default "div" | ||
*/ | ||
targetComponent?: string; |
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.
targetElementTag
& docs (see rootElementTag
immediately above)
@tgreen7 per your description notes, i think it's preferable here to not support |
@giladgray Thanks, that sounds fine. |
@giladgray How long do you think it will be before this fix gets into the release? |
@tgreen7 a day or two, will be cutting full 2.0.0 this week |
@giladgray awesome thanks! |
Fixes #2250
Checklist
Changes proposed in this pull request:
With the
targetElementTag
prop passed toPopover
orTooltip
you can change the component from a<div>
to a<g>
(or anything). This makes Tooltips work inside<svg>
. ThetargetComponent
prop is passed to react-popper as the component prop.Reviewers should focus on:
Popover
targetElementTag
. This should be able to be astring
or anJSX.Element
see floating-ui/react-popper#107 but I was not sure how to get typescript to allow both.NEED HELP: adding tests and fixing prop types to allow JSX.Element. As is solves issue #2250