tooltip component #1550
tooltip component #1550
Conversation
WalkthroughA new tooltip component system has been implemented in the frontend, introducing a comprehensive solution for displaying tooltips using React and Floating UI. The implementation includes a flexible Changes
Sequence DiagramsequenceDiagram
participant User
participant TooltipTrigger
participant TooltipContent
participant FloatingUI
User->>TooltipTrigger: Hover/Focus
TooltipTrigger->>FloatingUI: Request Positioning
FloatingUI-->>TooltipContent: Calculate Placement
TooltipContent->>User: Display Tooltip
Poem
🪧 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
🧹 Nitpick comments (3)
frontend/app/element/tooltip.tsx (1)
43-49: Check arrow middleware offset
In lines 43-49, you apply an offset of 5px, then set the arrow with a 5px offset (line 188) to avoid the tooltip arrow overlapping the trigger. Verify that these values are cohesive for all placements. Consider making them configurable or referencing a single constant for maintainability.frontend/app/element/tooltip.stories.tsx (2)
43-81: Uncontrolled story usage
In lines 43-81, the Uncontrolled story demonstrates internal open-state handling. This clearly shows how the tooltip behaves without explicit external control. Consider adding a minimal delay or a comment reference if the usability of the tooltip changes with heavier content or transitions.
83-113: Ensure onOpenChange is documented
Lines 83-113 demonstrate a controlled approach and callonOpenChangefrom user clicks. This is valuable for complex usage scenarios. Consider adding a unique knobs/control in Storybook for the open state, so that users can see the effect of toggling the tooltip externally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app/element/tooltip.scss(1 hunks)frontend/app/element/tooltip.stories.tsx(1 hunks)frontend/app/element/tooltip.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/app/element/tooltip.scss
🔇 Additional comments (5)
frontend/app/element/tooltip.tsx (4)
31-42: Validate controlled vs uncontrolled state logic
Having both controlled and uncontrolled states in lines 31-42 is a great approach for flexibility. However, ensure that the open prop and initialOpen prop do not conflict. If both are provided, the controlled state on line 40 will override the uncontrolled state. This is intended but might cause confusion for users if not adequately documented.
81-89: Ensure consistent usage of TooltipContext
The check in lines 84-85 ensures that the consumer of the tooltip is properly nested. This pattern is solid for preventing misuse. Also make sure you handle any top-level usage that might attempt to render a tooltip outside of a provider.
98-139: Ref safety for child components
In lines 98-113, you correctly handle referencing logic for child components by merging the child’s ref with the Floating UI reference. This is very good. However, it's important to be cautious when children are React Fragments or portal-based elements, ensuring no errors occur if the child doesn't forward refs.
141-197: Arrow styling and separate concerns
In lines 141-197, the arrow's background is inherited from the tooltip (line 185). Consider a separate styling approach (like a pseudo-element) for the tooltip arrow, making arrow styling more flexible. Also, check if the arrow remains visible on lighter backgrounds.
frontend/app/element/tooltip.stories.tsx (1)
10-38: Comprehensive argTypes
Lines 10-38 define a wide range of controls (placement, className, etc.). This is an excellent practice, as it empowers non-technical stakeholders to experiment with the component in Storybook.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
frontend/app/element/tooltip.tsx (4)
22-30: Add JSDoc comments to improve documentation.Consider adding JSDoc comments to document the purpose and usage of each option in the TooltipOptions interface.
interface TooltipOptions { + /** Initial open state for uncontrolled mode */ initialOpen?: boolean; + /** Preferred placement of the tooltip */ placement?: Placement; + /** Controlled open state */ open?: boolean; + /** Additional CSS class names */ className?: string; + /** Whether to show the arrow pointer */ showArrow?: boolean; + /** Callback for open state changes */ onOpenChange?: (open: boolean) => void; }
91-96: Pass className to the wrapper element.The
classNameoption is not being applied to any element. Consider wrapping the children in a div with the provided className.export const Tooltip = ({ children, ...options }: { children: React.ReactNode } & TooltipOptions) => { const tooltip = useTooltip(options); - return <TooltipContext.Provider value={tooltip}>{children}</TooltipContext.Provider>; + return ( + <TooltipContext.Provider value={tooltip}> + <div className={options.className}>{children}</div> + </TooltipContext.Provider> + ); };
98-139: Consider improvements to the trigger component.
- The "grey" className is hardcoded for the default button, making it less flexible.
- The ref handling logic is complex and could benefit from being extracted into a separate utility.
+// Extract ref handling to a utility +const createRefHandler = ( + setReference: (node: HTMLElement | null) => void, + propRef: React.ForwardedRef<HTMLElement>, + childRef?: React.Ref<HTMLElement> +) => (node: HTMLElement | null) => { + setReference(node); + if (typeof propRef === "function") propRef(node); + else if (propRef) (propRef as React.MutableRefObject<HTMLElement | null>).current = node; + if (childRef) { + if (typeof childRef === "function") childRef(node); + else (childRef as React.MutableRefObject<HTMLElement | null>).current = node; + } +}; export const TooltipTrigger = React.forwardRef<HTMLElement, React.HTMLProps<HTMLElement> & { asChild?: boolean; + buttonClassName?: string; // Add prop for button className }>( - function TooltipTrigger({ children, asChild = false, ...props }, propRef) { + function TooltipTrigger({ children, asChild = false, buttonClassName = "grey", ...props }, propRef) { const state = useTooltipState(); - const setRefs = (node: HTMLElement | null) => { - state.refs.setReference(node); - if (typeof propRef === "function") propRef(node); - else if (propRef) (propRef as React.MutableRefObject<HTMLElement | null>).current = node; - - if (React.isValidElement(children) && children.type !== React.Fragment && "ref" in children) { - if (typeof children.ref === "function") children.ref(node); - else (children.ref as React.MutableRefObject<HTMLElement | null>).current = node; - } - }; + const childRef = React.isValidElement(children) && children.type !== React.Fragment && "ref" in children + ? (children as any).ref + : undefined; + const setRefs = createRefHandler(state.refs.setReference, propRef, childRef); if (asChild && React.isValidElement(children)) { // ... rest of the code } return ( <Button - className="grey" + className={buttonClassName} ref={setRefs} // ... rest of the code >
1-197: Overall architectural assessment is positive.The tooltip implementation is well-structured and follows React best practices. It provides a flexible API with both controlled and uncontrolled modes, uses modern React patterns, and leverages the powerful Floating UI library for positioning. The component composition is clean, with clear separation of concerns between the hook, context, and individual components.
Consider adding:
- Unit tests to verify the behavior in both controlled and uncontrolled modes
- Storybook stories to showcase different configurations
- Performance optimization by memoizing callbacks and values where appropriate
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/element/tooltip.scss(1 hunks)frontend/app/element/tooltip.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/element/tooltip.scss
🔇 Additional comments (2)
frontend/app/element/tooltip.tsx (2)
1-21: LGTM: Imports and license header are properly structured.
The imports are well-organized and the license header is correctly formatted.
76-89: LGTM: Well-implemented context and state management.
The context implementation follows React best practices with proper error handling for missing context providers.
| export const useTooltip = ({ | ||
| initialOpen = false, | ||
| placement = "top", | ||
| open: controlledOpen, | ||
| onOpenChange: setControlledOpen, | ||
| }: TooltipOptions = {}) => { | ||
| const arrowRef = React.useRef(null); | ||
| const [uncontrolledOpen, setUncontrolledOpen] = React.useState(initialOpen); | ||
|
|
||
| const open = controlledOpen ?? uncontrolledOpen; | ||
| const setOpen = setControlledOpen ?? setUncontrolledOpen; | ||
|
|
||
| const data = useFloating({ | ||
| placement, | ||
| open, | ||
| onOpenChange: setOpen, | ||
| whileElementsMounted: autoUpdate, | ||
| middleware: [offset(5), flip(), shift(), arrow({ element: arrowRef })], | ||
| }); | ||
|
|
||
| const context = data.context; | ||
|
|
||
| const hover = useHover(context, { | ||
| move: false, | ||
| enabled: controlledOpen == null, | ||
| }); | ||
| const focus = useFocus(context, { | ||
| enabled: controlledOpen == null, | ||
| }); | ||
| const dismiss = useDismiss(context); | ||
| const role = useRole(context, { role: "tooltip" }); | ||
|
|
||
| const interactions = useInteractions([hover, focus, dismiss, role]); | ||
|
|
||
| return React.useMemo( | ||
| () => ({ | ||
| open, | ||
| setOpen, | ||
| arrowRef, | ||
| ...interactions, | ||
| ...data, | ||
| }), | ||
| [open, setOpen, arrowRef, interactions, data] | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing unused options and optimizing arrow middleware.
- The
classNameandshowArrowoptions from TooltipOptions are not being used. - The arrow middleware is always added regardless of the
showArrowoption.
const data = useFloating({
placement,
open,
onOpenChange: setOpen,
whileElementsMounted: autoUpdate,
- middleware: [offset(5), flip(), shift(), arrow({ element: arrowRef })],
+ middleware: [
+ offset(5),
+ flip(),
+ shift(),
+ ...(options.showArrow ? [arrow({ element: arrowRef })] : []),
+ ],
});Committable suggestion skipped: line range outside the PR's diff.
| export const TooltipContent = React.forwardRef<HTMLDivElement, React.HTMLProps<HTMLDivElement>>( | ||
| function TooltipContent(props, propRef) { | ||
| const state = useTooltipState(); | ||
|
|
||
| const ref = React.useMemo(() => { | ||
| const setRef = (node: HTMLDivElement | null) => { | ||
| state.refs.setFloating(node); // Use `refs.setFloating` from `useFloating` | ||
| if (typeof propRef === "function") propRef(node); | ||
| else if (propRef) (propRef as React.MutableRefObject<HTMLDivElement | null>).current = node; | ||
| }; | ||
| return setRef; | ||
| }, [state.refs.setFloating, propRef]); | ||
|
|
||
| const { x: arrowX, y: arrowY } = state.middlewareData.arrow ?? {}; | ||
|
|
||
| const staticSide = | ||
| { | ||
| top: "bottom", | ||
| right: "left", | ||
| bottom: "top", | ||
| left: "right", | ||
| }[state.placement.split("-")[0]] ?? ""; | ||
|
|
||
| return ( | ||
| <FloatingPortal> | ||
| {state.open && ( | ||
| <div | ||
| ref={ref} | ||
| style={{ | ||
| position: state.strategy, | ||
| top: state.y ?? 0, | ||
| left: state.x ?? 0, | ||
| visibility: state.x == null ? "hidden" : "visible", | ||
| ...props.style, | ||
| }} | ||
| {...state.getFloatingProps(props)} | ||
| > | ||
| {props.children} | ||
| <div | ||
| ref={state.arrowRef} | ||
| style={{ | ||
| position: "absolute", | ||
| width: "10px", | ||
| height: "10px", | ||
| background: "inherit", | ||
| left: arrowX != null ? `${arrowX}px` : "", | ||
| top: arrowY != null ? `${arrowY}px` : "", | ||
| [staticSide]: "-5px", | ||
| transform: "rotate(45deg)", | ||
| }} | ||
| /> | ||
| </div> | ||
| )} | ||
| </FloatingPortal> | ||
| ); | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility and respect showArrow option.
- The arrow is always rendered regardless of the
showArrowoption. - Missing ARIA attributes for better accessibility.
export const TooltipContent = React.forwardRef<HTMLDivElement, React.HTMLProps<HTMLDivElement>>(
function TooltipContent(props, propRef) {
const state = useTooltipState();
+ const { showArrow } = state; // Get showArrow from state
// ... rest of the code
return (
<FloatingPortal>
{state.open && (
<div
ref={ref}
+ role="tooltip"
+ aria-label={typeof props.children === 'string' ? props.children : undefined}
style={{
position: state.strategy,
top: state.y ?? 0,
left: state.x ?? 0,
visibility: state.x == null ? "hidden" : "visible",
...props.style,
}}
{...state.getFloatingProps(props)}
>
{props.children}
+ {showArrow && (
<div
ref={state.arrowRef}
style={{
position: "absolute",
width: "10px",
height: "10px",
background: "inherit",
left: arrowX != null ? `${arrowX}px` : "",
top: arrowY != null ? `${arrowY}px` : "",
[staticSide]: "-5px",
transform: "rotate(45deg)",
}}
/>
+ )}
</div>
)}
</FloatingPortal>
);
}
);📝 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.
| export const TooltipContent = React.forwardRef<HTMLDivElement, React.HTMLProps<HTMLDivElement>>( | |
| function TooltipContent(props, propRef) { | |
| const state = useTooltipState(); | |
| const ref = React.useMemo(() => { | |
| const setRef = (node: HTMLDivElement | null) => { | |
| state.refs.setFloating(node); // Use `refs.setFloating` from `useFloating` | |
| if (typeof propRef === "function") propRef(node); | |
| else if (propRef) (propRef as React.MutableRefObject<HTMLDivElement | null>).current = node; | |
| }; | |
| return setRef; | |
| }, [state.refs.setFloating, propRef]); | |
| const { x: arrowX, y: arrowY } = state.middlewareData.arrow ?? {}; | |
| const staticSide = | |
| { | |
| top: "bottom", | |
| right: "left", | |
| bottom: "top", | |
| left: "right", | |
| }[state.placement.split("-")[0]] ?? ""; | |
| return ( | |
| <FloatingPortal> | |
| {state.open && ( | |
| <div | |
| ref={ref} | |
| style={{ | |
| position: state.strategy, | |
| top: state.y ?? 0, | |
| left: state.x ?? 0, | |
| visibility: state.x == null ? "hidden" : "visible", | |
| ...props.style, | |
| }} | |
| {...state.getFloatingProps(props)} | |
| > | |
| {props.children} | |
| <div | |
| ref={state.arrowRef} | |
| style={{ | |
| position: "absolute", | |
| width: "10px", | |
| height: "10px", | |
| background: "inherit", | |
| left: arrowX != null ? `${arrowX}px` : "", | |
| top: arrowY != null ? `${arrowY}px` : "", | |
| [staticSide]: "-5px", | |
| transform: "rotate(45deg)", | |
| }} | |
| /> | |
| </div> | |
| )} | |
| </FloatingPortal> | |
| ); | |
| } | |
| ); | |
| export const TooltipContent = React.forwardRef<HTMLDivElement, React.HTMLProps<HTMLDivElement>>( | |
| function TooltipContent(props, propRef) { | |
| const state = useTooltipState(); | |
| const { showArrow } = state; // Get showArrow from state | |
| const ref = React.useMemo(() => { | |
| const setRef = (node: HTMLDivElement | null) => { | |
| state.refs.setFloating(node); // Use `refs.setFloating` from `useFloating` | |
| if (typeof propRef === "function") propRef(node); | |
| else if (propRef) (propRef as React.MutableRefObject<HTMLDivElement | null>).current = node; | |
| }; | |
| return setRef; | |
| }, [state.refs.setFloating, propRef]); | |
| const { x: arrowX, y: arrowY } = state.middlewareData.arrow ?? {}; | |
| const staticSide = | |
| { | |
| top: "bottom", | |
| right: "left", | |
| bottom: "top", | |
| left: "right", | |
| }[state.placement.split("-")[0]] ?? ""; | |
| return ( | |
| <FloatingPortal> | |
| {state.open && ( | |
| <div | |
| ref={ref} | |
| role="tooltip" | |
| aria-label={typeof props.children === 'string' ? props.children : undefined} | |
| style={{ | |
| position: state.strategy, | |
| top: state.y ?? 0, | |
| left: state.x ?? 0, | |
| visibility: state.x == null ? "hidden" : "visible", | |
| ...props.style, | |
| }} | |
| {...state.getFloatingProps(props)} | |
| > | |
| {props.children} | |
| {showArrow && ( | |
| <div | |
| ref={state.arrowRef} | |
| style={{ | |
| position: "absolute", | |
| width: "10px", | |
| height: "10px", | |
| background: "inherit", | |
| left: arrowX != null ? `${arrowX}px` : "", | |
| top: arrowY != null ? `${arrowY}px` : "", | |
| [staticSide]: "-5px", | |
| transform: "rotate(45deg)", | |
| }} | |
| /> | |
| )} | |
| </div> | |
| )} | |
| </FloatingPortal> | |
| ); | |
| } | |
| ); |
Summary by CodeRabbit
New Features
Bug Fixes