Skip to content

[BUG] Changing the content prop to a falsey value doesn't update the displayed content #875

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

Closed
phoenixeliot opened this issue Dec 22, 2022 · 9 comments · Fixed by #873
Closed
Labels

Comments

@phoenixeliot
Copy link

phoenixeliot commented Dec 22, 2022

Describe the bug
If you control the content prop with conditional logic, only the initial value is used for the actual display content.

Version of Package
v5.3.0

To Reproduce
Create a tooltip like so:

const [state, setState] = useState(true);

return (
  <>
    <div id="some-id" onClick={() => setState(!state)}>text</div>
    <Tooltip anchorId="some-id" content={state ? 'some content' : ''}/>
  </>
)

Expected behavior
Changing the content prop should cause the displayed tooltip to change to '' (no tooltip). Instead, it will stay as "some content" in the above example regardless of the value of state

Screenshots
None for now.

Desktop (please complete the following information if possible or delete this section):

  • OS: macOS
  • Browser: Brave (Chromium-based)
  • Version 1.46.144

Additional context
This is a real blocker for me because if I try to use the content="." hack to fix #872 and use children for the conditional instead, then if my conditional returns null, then a tooltip is shown with that period in it instead of nothing, like I want. So there is no way to show conditional logic that might return null, so there is no way to conditionally disable a tooltip that I know of.

@phoenixeliot
Copy link
Author

phoenixeliot commented Dec 22, 2022

Update: Actually it turns out this only happens if the value you change it to is falsey; it doesn't obey the change then. I'll update the ticket to match.

@phoenixeliot phoenixeliot changed the title [BUG] Changing the content prop doesn't update the displayed content [BUG] Changing the content prop to a falsey value doesn't update the displayed content Dec 22, 2022
@phoenixeliot
Copy link
Author

phoenixeliot commented Dec 22, 2022

For now the best workaround I have is that if I need to not show the tooltip, I have to just conditionally not render the tooltip component at all, but this is very inconvenient for cases where I have a multi-chain of conditions in the content but also need to use that conditional to determine whether to even render the thing or not.

eg, this is annoying to have to do:

{!(!someVar || someCheck1 || someCheck2) && (
  <Tooltip
    anchorId="some-button"
    content={
      !someVar
        ? 'You need to turn on the thing'
        : someCheck1
        ? "You don't have enough rubies."
        : someCheck2
        ? 'Nothing is for sale here yet.'
        : '' // doesn't cause the previous message to be removed if this expression changes to ''
    }
  />
)}

@gabrieljablonski
Copy link
Member

gabrieljablonski commented Dec 22, 2022

Thanks for testing it out. The fix is pretty simple, the latest commit in #873 should take care of the problem.

The PR will probably be merged tomorrow, so hang on until then. Thanks for the help.

@phoenixeliot
Copy link
Author

Just checking, would this same problem still occur with the html field too then, if that's the only field I'm using?
Like:

{!(!someVar || someCheck1 || someCheck2) && (
  <Tooltip
    anchorId="some-button"
    html={
      !someVar
        ? 'You need to turn on the thing'
        : someCheck1
        ? "You don't have enough rubies."
        : someCheck2
        ? 'Nothing is for sale here yet.'
        : '' // doesn't cause the previous message to be removed if this expression changes to ''
    }
  />
)}

@phoenixeliot
Copy link
Author

phoenixeliot commented Dec 22, 2022

I wonder if storing it internally as a boolean (isHtmlContent) and a maybe-html string will run into problems with this and it may just be simpler to have something like this in place of here, and pass through html and content instead: https://github.com/ReactTooltip/react-tooltip/blob/master/src/components/Tooltip/Tooltip.tsx#L305

{children || (html ? <TooltipContent content={html as string} /> : content)}

@gabrieljablonski
Copy link
Member

gabrieljablonski commented Dec 22, 2022

Just checking, would this same problem still occur with the html field too then, if that's the only field I'm using?

That won't be an issue, since whenever html changes, the tooltip content is set to content first.

As for having a separate internal state variable for storing the html content, that is indeed a good idea. We'll leave it as-is for now, but you can be sure the next time we run into any issues regarding this, it will definitely get refactored.

@phoenixeliot
Copy link
Author

Ah, right, I suppose so. Seems weird to thrash the "isHtmlContent" variable back and forth but if it works, then shrug.

@gabrieljablonski
Copy link
Member

For sure. But since things seem to be working fine as of right now, best not to mess with it. Let sleeping dogs lie or however the saying goes haha

@phoenixeliot
Copy link
Author

Just tested using your branch in my app, and everything works as expected now :). Thanks for the fast turnaround! Eager to plug in the next release tomorrow and get my feature out the door.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants