Fixed Tooltip gets hidden in “Fast & Lightweight” section bug - #247
Conversation
…fault value as right ( previously added )
|
@codevory is attempting to deploy a commit to the Ryan Millard's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughTooltip component API and behavior updated: added default props for Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant User
end
rect rgba(200,255,200,0.5)
participant Child as TargetElement
end
rect rgba(255,200,200,0.5)
participant TooltipWrapper as Tooltip
end
rect rgba(255,255,200,0.5)
participant ReactTooltip
end
User->>Child: hover / focus / touch
Child->>TooltipWrapper: DOM event (attributes from cloneElement)
TooltipWrapper->>TooltipWrapper: determine id (id ?? `tooltip-${reactId}`), detect touch, check dynamicPositioning
TooltipWrapper->>ReactTooltip: open with id, place=position, positionStrategy="absolute"
ReactTooltip-->>User: render tooltip UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/Tooltip.jsx`:
- Around line 136-139: The PropTypes for Tooltip are incorrect: remove the
isRequired flag from id, dynamicPositioning, and position in the
Tooltip.propTypes declaration to match the default parameters provided in the
function signature (so id, dynamicPositioning, and position become optional in
the propTypes). Locate the propTypes object (Tooltip.propTypes) in
src/components/Tooltip.jsx and update the entries for id, dynamicPositioning,
and position to use PropTypes.string / PropTypes.bool / PropTypes.string without
.isRequired.
- Around line 5-10: The component calls useId() inside the default parameter
(violates rules-of-hooks); move the hook call into the Tooltip component body
(e.g., const resolvedId = id ?? useId()) and replace all id references with
resolvedId (update the three places: the attribute "data-tooltip-id" and the JSX
attributes currently noted). Also update Tooltip.propTypes to mark id, position,
and dynamicPositioning as optional (not required) to reflect the defaults.
🧹 Nitpick comments (1)
src/components/Tooltip.jsx (1)
121-125: Normalizepositionprop before passing to ReactTooltip.React-tooltip's
placeprop is case-sensitive and only accepts lowercase values (top,right,bottom,left, and hyphenated variants liketop-start). Line 122 passespositiondirectly without validation. Current usages in the codebase pass compliant lowercase values, but the component accepts any string via PropTypes and has no guard. Consider normalizing to lowercase and restricting to allowed values to prevent future bugs if callers pass capitalized or invalid values.♻️ Suggested guard
+ const normalizedPosition = ["top", "right", "bottom", "left"].includes( + (position || "").toLowerCase(), + ) + ? position.toLowerCase() + : "right";- place={position} + place={normalizedPosition}
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
2 participants
What was changed & why
Added position props to Tooltip component with default value as "right" (previously defined) and position="Top" to two of the cards at " Fast & Lightweight " section .
I have added position of Tooltip content as "Top" for both cards . Adding position to single card ie. card which was having bug, would look unprofessional.
Fixes: #160
Changes
Changed the position of two cards ( on Home page -> “Fast & Lightweight” ) from "right" --> "Top", passed position as props.
Description
I have fixed the bug "Tooltip gets hidden in “Fast & Lightweight” section .
I tried to place the Tooltipt content to left / right but i guess due to less space around the cards, the Tooltip content get shifted to other positions via dynamicPositioning fallbackPlacements={dynamicPositioning ? ["bottom", "top", "left"] : []}.
Iam waiting for your feedback & any suggestion.
Testing & Verification
Not written tests yet but will soon once reviewed by maintainer.
Additional Resources
Summary by CodeRabbit
New Features
UI Copy
Tweaks
✏️ Tip: You can customize this high-level summary in your review settings.