-
-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #577: [New Feature] Added Multiple direction support for Tooltip … #601
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related issues
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/ui/Tooltip/Tooltip.tsx (1)
25-25
: Fix spacing around the assignment operator.ESLint correctly identified a spacing issue.
-const Tooltip = ({ children, label="hello", placement = 'top', ...props }: TooltipProps) => { +const Tooltip = ({ children, label = "hello", placement = 'top', ...props }: TooltipProps) => {🧰 Tools
🪛 eslint
[error] 25-25: Operator '=' must be spaced.
(space-infix-ops)
src/components/ui/Tooltip/stories/Tooltip.stories.js (1)
11-13
: Enhance stories to showcase all placement options.While the current example demonstrates the 'left' placement, it would be valuable to showcase all cardinal and compound directions to help users understand the full capabilities.
Consider adding:
- A story for each cardinal direction (top, right, bottom, left)
- Examples of compound directions (e.g., top-start, bottom-end)
- Interactive controls to let users experiment with different placements
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/components/tools/Popper/Popper.tsx
(1 hunks)src/components/ui/Tooltip/Tooltip.tsx
(1 hunks)src/components/ui/Tooltip/stories/Tooltip.stories.js
(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/ui/Tooltip/Tooltip.tsx
[error] 25-25: Operator '=' must be spaced.
(space-infix-ops)
🔇 Additional comments (3)
src/components/tools/Popper/Popper.tsx (2)
24-36
: LGTM! Comprehensive placement options added.
The expanded placement options align well with Floating UI's capabilities and the PR objectives. The type definition correctly includes all cardinal and compound directions.
Line range hint 6-7
: Address collision handling TODOs before merging.
The TODO comments indicate potential issues with collision handling. This could affect the reliability of the new placement options, especially in constrained viewport scenarios.
Let's verify the collision handling behavior:
Consider:
- Testing edge cases with viewport constraints
- Documenting the expected fallback behavior
- Implementing proper collision resolution strategies
src/components/ui/Tooltip/Tooltip.tsx (1)
9-21
: LGTM! Placement type matches Popper component.
The placement options are correctly synchronized with the Popper component's type definition.
@@ -8,7 +8,7 @@ export default { | |||
component: Tooltip, | |||
render: (args) => <SandboxEditor> | |||
<ScrollPlayground> | |||
<Tooltip className="text-gray-1000" label="This is a tooltip"> | |||
<Tooltip className="text-gray-1000" label="This is a tooltip" placement='left'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wanna add more examples to test it out? Just for sanity sake
This PR adds support for cardinal and compound directions in the Tooltip component by extending the placement prop. The supported directions include:
Cardinal Directions:
top
,bottom
,left
,right
.Compound Directions:
top-start
,top-end
,bottom-start
,bottom-end
,left-start
,left-end
,right-start
,right-end
.Key Changes:
Updated
Popper
andTooltip
components to support these directions using the placement prop.Integrated Floating UI middleware to handle placements dynamically, ensuring proper positioning based on viewport constraints.
Tested the functionality with various placements and alignment strategies.
Summary by CodeRabbit
New Features
Popper
andTooltip
components, now supporting twelve positioning values.label
andplacement
props in theTooltip
component, enhancing configurability.Bug Fixes
Tooltip
stories to reflect the newplacement
prop for accurate rendering.