Added enforceDelayShow prop. (Fixes #1235) #1236
Conversation
This will make the delayShow prop to always be enforced even if the tooltip instance is rendered. This can be used for when you only have one tooltip instance, but multiple tooltip anchors, and want it to always take the delayShow amount of time before it is shown.
WalkthroughThe changes introduce a new optional prop Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Tooltip/TooltipTypes.d.ts (1)
161-161: Add JSDoc documentation for the new propThe type definition is correct and well-placed near other delay-related props. However, consider adding JSDoc documentation to explain its purpose and relationship with
delayShow.+ /** + * @description When true, ensures delayShow is always respected, even if the tooltip was previously rendered + */ enforceDelayShow?: booleansrc/components/Tooltip/Tooltip.tsx (1)
Line range hint
263-267: Add a comment explaining the enforceDelayShow behavior.While the logic is correct, adding a comment would help future maintainers understand the purpose of
enforceDelayShowand its impact on delay behavior.+ // When enforceDelayShow is true, always apply the delay + // even if the tooltip was previously rendered if (rendered && !enforceDelayShow) { // if the tooltip is already rendered, ignore delay handleShow(true) return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Tooltip/Tooltip.tsx(2 hunks)src/components/Tooltip/TooltipTypes.d.ts(1 hunks)
🔇 Additional comments (2)
src/components/Tooltip/Tooltip.tsx (2)
71-71: LGTM! Well-defined prop with clear naming.
The new prop enforceDelayShow is properly declared with a sensible default value that maintains backward compatibility.
71-71: Implementation successfully addresses the PR objectives.
The changes effectively solve issue #1235 by:
- Adding a new opt-in prop
enforceDelayShow - Maintaining backward compatibility with default
falsevalue - Implementing the delay enforcement logic correctly
Also applies to: 263-267
|
I went back to version 5.24.0. And I assume I would get the same issues with my changes that I am proposing. That the tooltip blinks between the old anchor value and the new anchor value. |
Tried approving the beta release so you could use it in your package until we have time to review this, but it didn't work. But since 5.24.0 apparently solved it for you, it's fine for now then. Thanks for the contribution, will go over it when I have the time. |
|
I think you misunderstood me. What I'm trying to say is that 5.24.0 and my proposed changes will probably behave the same. 5.24.0 has the issue that if you hover between anchors that have the same tooltip instance, the tooltip will first disappear and reappear on the new anchor. I will see if I can find a related issue to link to you, since I hope that someone else can explain it better than me : ) Edit: |
|
Hi! |
|
@Ville-Wennerlund sorry haven't had much time to go over stuff here. Your change is simple enough, that I don't think it should break anything. But please update the options table on the docs, and I'll do a quick test later and possibly do a release with the new option. |
I'm referring to this comment. |
This will make the delayShow prop to always be enforced even if the tooltip instance is rendered.
This can be used for when you only have one tooltip instance, but multiple tooltip anchors, and want it to always take the delayShow amount of time before it is shown.
Summary by CodeRabbit
enforceDelayShowfor theTooltipcomponent, allowing users to control the display timing of tooltips with more precision.