-
Notifications
You must be signed in to change notification settings - Fork 189
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
chore(components): refactor InteractivePopover
to take a type argument for the trigger element
#6495
Conversation
@@ -42,12 +42,12 @@ const closeButtonStyles = css({ | |||
right: spacing[2], | |||
}); | |||
|
|||
type InteractivePopoverProps = { | |||
type InteractivePopoverProps<TriggerElement extends HTMLElement> = { |
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.
This could have followed a pattern and be named T
but it felt a bit too vague 🤔
406964e
to
a2ccca2
Compare
@@ -102,7 +102,7 @@ export default function ConnectionsFilterPopover({ | |||
onMouseLeave={handleButtonMouseLeave} | |||
active={open} | |||
aria-label="Filter connections" | |||
ref={ref as React.Ref<unknown>} | |||
ref={ref} |
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.
I believe it's possible to pass the ref
here without having to provide an explicit type parameter to the InteractivePopover
above, is possible because the IconButton
use the forwardRef
utility internally.
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.
Neat, thanks for the follow up to clean this up!
a2ccca2
to
02fd75e
Compare
Description
As a follow-up to this #6486 (comment), I suggest adding a type argument to the
InteractivePopover
to specify the concreteHTMLElement
used when declaring the type of theref
passed through thetrigger
callback.We can't just pass
React.Ref<HTMLElement>
as that (as an example) isn't possible to pass viaref
to a<button />
:☝️ yields this TS error related to the
ref
prop:Checklist
Motivation and Context
Open Questions
Dependents
Types of changes