Skip to content
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): Tooltip target element option #356

Merged
merged 7 commits into from
Mar 31, 2017

Conversation

nlrowe
Copy link
Contributor

@nlrowe nlrowe commented Mar 14, 2017

Added the option to pass a DOM element to the tooltip target. The 'getTarget' function was also used for the tether config target to maintain post component mounting behavior. If this is not a concern, it can just reference this._target that is created on mount.

Addresses #337

nlrowe added 3 commits March 11, 2017 22:38
Changed the tooltip target property to also allow for DOM elements.
@mawburn
Copy link

mawburn commented Mar 14, 2017

This is the same concern. @nlrowe and I work together.

src/Tooltip.js Outdated
getTetherConfig() {
const attachments = getTetherAttachments(this.props.placement);
return {
...defaultTetherConfig,
...attachments,
target: '#' + this.props.target,
target: this.getTarget(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot that I used the string selector because I wasn't sure if the target element would be available at the time getTetherConfig is called. Tether also accepts a function as target. This might be safer as:

target: this.getTarget

this._target = this.getTarget() in componentDidMount should be fine since it's mounted.

@eddywashere
Copy link
Member

Good callout for this._target. It should be fine to just reference that 👍

@nlrowe
Copy link
Contributor Author

nlrowe commented Mar 16, 2017

If we change getTetherConfig to use this._target, the component fails if it is initialized with isOpen set to true as the render actually executes with an undefined this._target when it is called before the componentDidMount.

The check in render can be modified to handle for that:

    if (!(this.props.isOpen && this._target)) {
      return null;
    }

Or, we can use the function and it will go get the element on its own.

Which would you prefer good sir?

@eddywashere
Copy link
Member

@nlrowe ah I was hoping the timing/lifecycle would be fine. Good to know, I think we should pass in the function instead. Thanks for working through this!

nlrowe and others added 3 commits March 16, 2017 12:35
Pass getTarget function instead of the target retrieved in
componentDidMount as it is not always available at render.
@eddywashere eddywashere merged commit 2023036 into reactstrap:master Mar 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants