resolve --rt-transition-show-delay closer to tooltip for js timer #1255
Conversation
Changing `--rt-transition-show-delay` anywhere except in head or on body were not be picked up by show/hide timeouts causing out animations to be skipped entirely by removing the tooltip from the DOM. this change evaluates the variable on the tooltip its self so that the property can be honored lower in the tree.
WalkthroughThe tooltip component now computes the transition delay from the tooltip’s own DOM node (via tooltipRef) instead of always using document.body, with a fallback to document.body. No exports or public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Trigger as TooltipTrigger
participant Tooltip as Tooltip Component
participant DOM as DOM/CSS
User->>Trigger: Hover/Focus
Trigger->>Tooltip: Show tooltip
Tooltip->>DOM: Render tooltip element
rect rgba(173,216,230,0.25)
Note over Tooltip: Determine transition delay
Tooltip->>Tooltip: Has tooltipRef.current?
alt Ref available
Tooltip->>DOM: getComputedStyle(tooltipRef.current)["--rt-transition-show-delay"]
else Fallback
Tooltip->>DOM: getComputedStyle(document.body)["--rt-transition-show-delay"]
end
end
Tooltip->>DOM: Apply delay to hide transition
User-->>Trigger: Unhover/Blur
Trigger->>Tooltip: Hide tooltip (with computed delay)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
🧹 Nitpick comments (2)
src/components/Tooltip/Tooltip.tsx (2)
239-251: Optional: prefer nullish coalescing and support a dedicated closing varMinor polish and future-proofing:
- Use
??over||for clarity with nullable refs.- If you later introduce
--rt-transition-closing-delay, honor it first while keeping full backward compatibility.Apply this diff:
- const style = getComputedStyle(tooltipRef.current || document.body) - const transitionShowDelay = cssTimeToMs(style.getPropertyValue('--rt-transition-show-delay')) + const el = tooltipRef.current ?? document.body + const style = getComputedStyle(el) + const closingVar = + style.getPropertyValue('--rt-transition-closing-delay').trim() || + style.getPropertyValue('--rt-transition-show-delay') + const transitionShowDelay = cssTimeToMs(closingVar)
239-251: Test suggestion: assert element-scoped var affects unmount timingAdd a test that sets
--rt-transition-show-delayon the tooltip node (notbody) and asserts the hide fallback waits that duration before unmounting (e.g., via fake timers andonTransitionEndnot firing). I can sketch the test if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/components/Tooltip/Tooltip.tsx(1 hunks)
🔇 Additional comments (1)
src/components/Tooltip/Tooltip.tsx (1)
239-241: Compute CSS var from the tooltip element — LGTMReading the custom property from
tooltipRef.current(with a safe fallback) fixes the scoping bug described in the PR and aligns JS timers with CSS animations.
Changing
--rt-transition-show-delayanywhere except in head or on body were not be picked up by show/hide timeouts causing out animations to be skipped entirely by removing the tooltip from the DOM when not using default positioned styles. This change evaluates the variable on the tooltip its self so that the property can be honored lower in the tree, which is already is for the animations, just not for the js timers.Motivation: Using in a project intended to be embedded in other GUIs that should not modify the the DOM outside of its parent. I can't find a compelling reason why this computed style needs to be pinned to the document.body scope.
note: Either the naming of these variables are confusing or its possibly just using the wrong css variable here,
--rt-transition-show-delayis used to delay unmounting when hiding and also to control the length of the default fade-in animation. It appears that this should really use--rt-transition-closing-delayas that's what dictates the length of the fade out duration. being new to the codebase I didn't want to make any assumptions but I'd be happy to change this as part of this PR if desired.Summary by CodeRabbit