Feat: Make use of the newly refactored popper component to make the tooltip. Also, add new story. #807
Conversation
WalkthroughThis pull request enhances the 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
🧹 Nitpick comments (2)
src/components/ui/Tooltip/stories/Tooltip.stories.tsx (2)
20-32: LGTM! Well-structured story with clear demonstration of functionality.The renamed story with enhanced styling provides a better demonstration of the tooltip's capabilities.
Consider adding a brief comment explaining the grid layout configuration for better maintainability:
+ {/* Grid layout to showcase all placement variations */} <div className="grid grid-cols-[repeat(auto-fit,minmax(200px,1fr))] justify-center gap-3">
Line range hint
34-45: LGTM! Good addition demonstrating the new showArrow prop.The new story effectively showcases the tooltip without arrows while maintaining consistency with the WithArrow story.
Consider adding the same layout comment here for consistency:
+ {/* Grid layout to showcase all placement variations */} <div className="grid grid-cols-[repeat(auto-fit,minmax(200px,1fr))] justify-center gap-3">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ui/Tooltip/Tooltip.tsx(1 hunks)src/components/ui/Tooltip/stories/Tooltip.stories.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
src/components/ui/Tooltip/Tooltip.tsx (4)
10-12: LGTM! Well-structured prop additions.The new optional props
showArrowandclassNameenhance the component's flexibility while maintaining good TypeScript practices.
14-14: LGTM! Well-structured component signature.The updated signature properly handles the new props with appropriate defaults.
16-27: LGTM! Excellent implementation of compound components pattern.The refactored structure using
Popper.Root,Popper.Trigger, andPopper.Contentimproves composability and follows best practices for compound components.
33-33: LGTM! Good practice exporting the props type.Exporting
TooltipPropsenables type reuse and improves developer experience.src/components/ui/Tooltip/stories/Tooltip.stories.tsx (1)
7-7: LGTM! Appropriate import addition.The addition of
Primitiveimport supports the enhanced story implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/components/tools/Popper/fragments/PopperTrigger.tsx (2)
8-8: Add JSDoc comment for the asChild prop.The
asChildprop is a powerful composition feature that allows the component to be rendered as a different element. Adding documentation would help other developers understand its purpose and usage.+ /** When true, the trigger will render its child directly instead of wrapping it in a span */ asChild?: boolean
11-20: Consider adding type safety for children when asChild is true.When
asChildis true, the children should be a valid React element to ensure proper composition.- export default function PopperTrigger({ asChild, children, className = '', ...props }: PopperTriggerProps) { + export default function PopperTrigger({ asChild, children, className = '', ...props }: PopperTriggerProps) { + if (asChild && !isValidElement(children)) { + throw new Error('PopperTrigger: children must be a valid React element when asChild is true'); + }src/components/tools/Popper/context/PopperContext.tsx (2)
Line range hint
1-1: Create tracking issue for the TODO comment.The TODO comment about using native floating.tsx should be tracked in an issue for future follow-up.
Would you like me to create an issue to track this TODO item?
11-11: Add JSDoc comment for the showArrow context property.Document the purpose and usage of the
showArrowproperty in the context.+ /** Controls the visibility of the floating arrow element */ showArrow: boolean;src/components/tools/Popper/fragments/PopperContent.tsx (1)
9-10: Add JSDoc comment for the asChild prop.Similar to PopperTrigger, document the purpose of the
asChildprop.+ /** When true, the content will render its child directly instead of wrapping it in a div */ asChild?: boolean;src/components/tools/Popper/stories/Popper.stories.tsx (1)
Line range hint
17-37: Enhance story coverage.The story could be improved by:
- Adding examples of
asChildusage for both Trigger and Content- Demonstrating different arrow configurations
- Showing various placement options
Consider adding these additional stories:
export const WithCustomTrigger: Story = { render: () => ( <Popper.Root> <Popper.Trigger asChild> <button>Custom Button</button> </Popper.Trigger> <Popper.Content>Content</Popper.Content> </Popper.Root> ) }; export const WithoutArrow: Story = { render: () => ( <Popper.Root showArrow={false}> <Popper.Trigger>Trigger</Popper.Trigger> <Popper.Content>Content</Popper.Content> </Popper.Root> ) };src/components/tools/Popper/Popper.tsx (1)
Line range hint
11-11: Address or track the TODO comment about collision issues.The comment indicates that collisions aren't working as expected. This should be investigated and tracked properly.
Would you like me to help create an issue to track this collision-related bug?
src/components/tools/Popper/fragments/PopperRoot.tsx (1)
Line range hint
33-52: Consider optimizing middleware order for better performance.The middleware stack is configured with arrow, offset, flip, shift, and hide. While this works, the order could be optimized:
offsetshould typically come beforearrowas it affects the base positioningflipandshiftare computationally expensive and should come lastConsider reordering the middleware stack:
middleware: [ + offset(ARROW_HEIGHT + GAP), arrow({ element: arrowRef, padding: 4 }), - offset(ARROW_HEIGHT + GAP), hide({ strategy: 'referenceHidden' }), flip({ mainAxis: true, fallbackStrategy: 'initialPlacement' }), shift({ crossAxis: false }) ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/tools/Popper/Popper.tsx(1 hunks)src/components/tools/Popper/context/PopperContext.tsx(1 hunks)src/components/tools/Popper/fragments/PopperContent.tsx(1 hunks)src/components/tools/Popper/fragments/PopperRoot.tsx(3 hunks)src/components/tools/Popper/fragments/PopperTrigger.tsx(1 hunks)src/components/tools/Popper/stories/Popper.stories.tsx(1 hunks)
🔇 Additional comments (3)
src/components/tools/Popper/fragments/PopperContent.tsx (1)
21-24: Verify portal mounting point.The component uses
document.bodyas a fixed portal root. Consider making this configurable to support different mounting points.src/components/tools/Popper/Popper.tsx (1)
38-40: Verify the arrow visibility behavior across components.The changes show that arrow visibility is now managed at the
PopperRootlevel instead ofPopperContent. Let's verify this change is consistent across the component hierarchy.✅ Verification successful
Arrow visibility implementation is correctly managed
The arrow visibility is properly managed through React Context, with PopperRoot as the single source of truth. The implementation is consistent across the component hierarchy:
- PopperRoot provides the showArrow state via context
- PopperContent correctly consumes and respects this value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining showArrow prop usage in PopperContent ast-grep --pattern 'showArrow' src/components/tools/Popper/fragments/PopperContent.tsx # Check for any direct arrow-related DOM elements in PopperContent rg -l "FloatingArrow" src/components/tools/Popper/fragments/PopperContent.tsxLength of output: 413
Script:
#!/bin/bash # Check PopperRoot implementation ast-grep --pattern 'const PopperRoot' -A 30 # Look for type definitions containing showArrow rg 'interface.*\{[^}]*showArrow[^}]*\}' --type typescript # Find all showArrow usages across components rg 'showArrow' --type typescriptLength of output: 208
Script:
#!/bin/bash # Find PopperRoot implementation rg -l "PopperRoot" src/components/tools/Popper/ cat $(rg -l "PopperRoot" src/components/tools/Popper/) # Find showArrow usages and type definitions rg "showArrow" src/components/tools/Popper/ -A 2Length of output: 6535
src/components/tools/Popper/fragments/PopperRoot.tsx (1)
71-71: Ensure PopperContext type includes showArrow.The context value now includes
showArrow, but we should verify that the PopperContext type definition has been updated accordingly.✅ Verification successful
PopperContext type definition is correctly updated
The
PopperContextTypeinterface insrc/components/tools/Popper/context/PopperContext.tsxincludesshowArrowas a required boolean property, which matches its usage in the context provider.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if PopperContext type includes showArrow ast-grep --pattern 'interface PopperContextType { $$$ showArrow: boolean; $$$ }' src/components/tools/Popper/context/PopperContext.tsxLength of output: 142
Script:
#!/bin/bash # First verify the file exists fd "PopperContext.tsx" --type f # Then look for interface definition and showArrow in a more flexible way rg -A 10 "interface PopperContextType" --type ts rg "showArrow.*:" --type tsLength of output: 542
Popper was refactored to be more composable. See #800. Tooltip was just wrapping around the popper component before but now it will use popper sub-components like
Popper.TriggerandPopper.Content.A new story was also added.
Summary by CodeRabbit
New Features
Documentation