docs(tooltip): remove shouldBlockScroll prop #4539
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request involves removing the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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
Documentation and Community
|
![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)
packages/components/tooltip/src/use-tooltip.ts (1)
172-174: Consider improving readability of the condition.The implementation is correct, but the double negation in
!(shouldBlockScroll && isOpen)could be made more readable.Consider this alternative:
- usePreventScroll({ - isDisabled: !(shouldBlockScroll && isOpen), - }); + usePreventScroll({ + isDisabled: !isOpen || !shouldBlockScroll, + });packages/components/tooltip/stories/tooltip.stories.tsx (1)
139-158: Consider enhancing the demo content.The template effectively demonstrates the feature, but could be more informative.
Consider adding more descriptive content to help users understand the impact:
- content="Scrolling outside the tooltip is blocked" + content={ + <div> + <p>Scrolling outside is blocked</p> + <p className="text-xs">Try scrolling the page while this tooltip is open</p> + </div> + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/slow-queens-attend.md(1 hunks)packages/components/tooltip/src/use-tooltip.ts(4 hunks)packages/components/tooltip/stories/tooltip.stories.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: TypeScript
- GitHub Check: Build
🔇 Additional comments (5)
packages/components/tooltip/src/use-tooltip.ts (3)
11-16: LGTM! Import of usePreventScroll is appropriate.The addition of usePreventScroll from @react-aria/overlays is necessary for implementing the scroll blocking functionality.
71-75: LGTM! Property is well-documented.The shouldBlockScroll property is properly documented with JSDoc comments, including the default value.
128-128: LGTM! Default value matches documentation.The shouldBlockScroll property is correctly initialized with the documented default value of true.
.changeset/slow-queens-attend.md (1)
1-5: LGTM! Appropriate version bump.The patch version bump is correct for adding new functionality in a backward-compatible way.
packages/components/tooltip/stories/tooltip.stories.tsx (1)
420-425: LGTM! Export follows consistent pattern.The story export follows the established pattern in the file.
wingkwong
force-pushed
the
fix/eng-1141
branch
from
ac67ebd to
988e6ff
Compare
wingkwong
changed the title
Closes #3474
📝 Description
The doc mentions
shouldBlockScrollin Tooltip but the logic is not found. As suggested by Ryo, it's unnatural for a tooltip element that appears on hover to block scrolling. Hence, this PR is to remove it to avoid confusion.⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit
shouldBlockScrollproperty from Tooltip component documentation