feat: add arrow size property #1251
Conversation
WalkthroughThe changes introduce an Changes
Sequence Diagram(s)sequenceDiagram
participant Consumer
participant TooltipController
participant Tooltip
participant computeTooltipPosition
Consumer->>TooltipController: Pass arrowSize prop
TooltipController->>Tooltip: Forward arrowSize prop
Tooltip->>computeTooltipPosition: Call with arrowSize
computeTooltipPosition-->>Tooltip: Return position using arrowSize
Tooltip->>Tooltip: Set --rt-arrow-size CSS variable
Tooltip->>DOM: Render arrow with correct size and position
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
✨ Finishing Touches
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. 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: 1
🧹 Nitpick comments (3)
src/components/Tooltip/TooltipTypes.d.ts (1)
161-161: Consider adding documentation and type constraints.The
arrowSizeproperty addition looks good. Consider enhancing it with:
- JSDoc documentation for clarity:
arrowColor?: CSSProperties['backgroundColor'] + /** + * @description Size of the tooltip arrow in pixels. + * @default 8 + */ arrowSize?: number
- Type constraint to ensure positive values:
arrowSize?: number & { __brand: 'PositiveNumber' }Or consider runtime validation in the component to ensure sensible values (e.g., > 0).
src/components/TooltipController/TooltipControllerTypes.d.ts (1)
94-94: Consider adding documentation for consistency.The
arrowSizeproperty addition maintains good consistency with theITooltipinterface. Consider adding JSDoc documentation for user clarity:arrowColor?: CSSProperties['backgroundColor'] + /** + * @description Size of the tooltip arrow in pixels. + * @default 8 + */ arrowSize?: numberThis would align with the documentation style used elsewhere in the interface and provide clear guidance to developers.
src/components/Tooltip/Tooltip.tsx (1)
906-906: Consider input validation for arrowSize.The CSS variable is correctly set with pixel units. However, consider adding validation to ensure
arrowSizeis a positive number to prevent unexpected styling behavior.Example validation:
style={{ ...computedPosition.tooltipArrowStyles, background: arrowColor ? `linear-gradient(to right bottom, transparent 50%, ${arrowColor} 50%)` : undefined, - '--rt-arrow-size': `${arrowSize}px`, + '--rt-arrow-size': `${Math.max(0, arrowSize || 8)}px`, }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/test/__snapshots__/tooltip-attributes.spec.js.snapis excluded by!**/*.snapsrc/test/__snapshots__/tooltip-props.spec.js.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
src/components/Tooltip/Tooltip.tsx(5 hunks)src/components/Tooltip/TooltipTypes.d.ts(1 hunks)src/components/Tooltip/core-styles.module.css(1 hunks)src/components/Tooltip/styles.module.css(1 hunks)src/components/TooltipController/TooltipController.tsx(2 hunks)src/components/TooltipController/TooltipControllerTypes.d.ts(1 hunks)src/utils/compute-tooltip-position-types.d.ts(1 hunks)src/utils/compute-tooltip-position.ts(2 hunks)
🔇 Additional comments (7)
src/components/Tooltip/core-styles.module.css (1)
17-17: LGTM! Prevents arrow overlap with larger sizes.The
z-index: -1effectively addresses the overlap issue mentioned in the PR objectives, ensuring larger arrow sizes render behind the tooltip content rather than covering it.src/utils/compute-tooltip-position-types.d.ts (1)
13-13: Clean type definition addition.The optional
arrowSizeproperty withnumbertype is well-defined and consistent with the implementation that provides a default value.src/components/Tooltip/Tooltip.tsx (3)
71-71: Good default value and prop definition.The
arrowSize = 8default is reasonable and consistent with the positioning logic that previously used a hardcoded value of 4 (which equals 8/2).
349-349: Correct integration with positioning logic.The
arrowSizeprop is properly passed to bothcomputeTooltipPositioncalls, ensuring consistent arrow positioning for both virtual element and active anchor scenarios.Also applies to: 442-442
462-462: Proper dependency management.Adding
arrowSizeto the dependency array ensures the tooltip position is recalculated when the arrow size changes, which is necessary for accurate positioning.src/components/TooltipController/TooltipController.tsx (1)
61-61: Clean prop forwarding implementation.The
arrowSizeprop is correctly destructured and forwarded to the underlyingTooltipcomponent, following the established pattern for other props in the controller.Also applies to: 372-372
src/utils/compute-tooltip-position.ts (1)
19-19: Correct calculation logic and backward compatibility.The
arrowSize = 8default maintains backward compatibility since the previous hardcoded value was 4 (8/2 = 4). The calculationarrowSize / 2 + borderWidthcorrectly positions the arrow by using half the arrow size plus any border width.The mathematical logic makes sense: the arrow offset should be half the arrow size to center it properly relative to the tooltip edge.
Also applies to: 82-82
| width: var(--rt-arrow-size); | ||
| height: var(--rt-arrow-size); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add fallback value for CSS variable.
Consider adding a fallback value to ensure the arrow has a size even if the CSS variable is not set:
-.arrow {
- width: var(--rt-arrow-size);
- height: var(--rt-arrow-size);
-}
+.arrow {
+ width: var(--rt-arrow-size, 8px);
+ height: var(--rt-arrow-size, 8px);
+}This provides backwards compatibility and prevents potential layout issues if the variable is undefined.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| width: var(--rt-arrow-size); | |
| height: var(--rt-arrow-size); | |
| /* src/components/Tooltip/styles.module.css */ | |
| .arrow { | |
| width: var(--rt-arrow-size, 8px); | |
| height: var(--rt-arrow-size, 8px); | |
| } |
🤖 Prompt for AI Agents
In src/components/Tooltip/styles.module.css at lines 9 to 10, the CSS properties
width and height use the variable --rt-arrow-size without fallback values. To
fix this, add fallback values to both properties using the CSS var() function
syntax, for example var(--rt-arrow-size, 10px), so the arrow size defaults to a
specific value if the variable is not defined, preventing layout issues.
Fixes #1250
arrowSizeproperty, which controls positioning as well as a css variable used to style the height and width of the arrow elementz-indexof -1 so that it renders behind the tooltip content, otherwise larger arrow sizes cover the content.Summary by CodeRabbit
New Features
Style