-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(Tooltip,Popover): base component for Tooltip and Popover (#1022) #1181
feat(Tooltip,Popover): base component for Tooltip and Popover (#1022) #1181
Conversation
…trap#1022) Added a base component which can be shared between both Tooltip and Popover. Also add custom trigger for Popover component
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.
The logic within the wrapping component should be as generic as possible. Some of the things in the code now can be cleaned up to be more generic.
} | ||
this._target.addEventListener('keydown', this.onEscKeyDown, true); | ||
} | ||
} else if (this.props.componentType === 'tooltip') { |
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 was thinking that Tooltip
can just provide triggers
to avoid specific implementation logic here.
Same with Popover
below.
this._hideTimeout = undefined; | ||
} | ||
|
||
handleDocumentClick(e) { |
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 think the logic between tooltip and popover is really the same but it was just implemented differently in the original components. It looks like they do the opposite checks to perform the opposite action.
There is some extra stuff on the second condition more specific to this._popover
, but it is just checking that the content is not within the popover. A similar check is probably needed for tooltip or made into another prop to control clicks within the "add element".
The goal is for this to be as generic as possible.
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.
@TheSharpieOne This is done by passing a ref
as prop
down to the div
inside, right? Since Tooltip
is providing innerRef
as a prop
, how can I do the same for Tooltip
(since the function can change from the parent created by the user)?
superseded by #1222 |
Added a base component which can be shared between both Tooltip and
Popover. Also add custom trigger for Popover component