- Notifications
You must be signed in to change notification settings - Fork4.4k
fix: tooltip position on edge of screen#34518
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:release
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
- Added Popper.js modifiers to prevent tooltip from overflowing viewport edges- Enabled 'preventOverflow' to restrict tooltip within the viewport- Enabled 'flip' to automatically reposition tooltip if there is insufficient space- Docs:https://floating-ui.com/docs/flip#flipResolves:appsmithorg#34445
coderabbitaibot commentedJun 26, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
WalkthroughThe Changes
Assessment against linked 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 withCodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/packages/design-system/widgets-old/src/Tooltip/index.tsx (1 hunks)
Additional comments not posted (2)
app/client/packages/design-system/widgets-old/src/Tooltip/index.tsx (2)
78-84:Enhancements to Tooltip Positioning LogicThe modifications to the
preventOverflowandflipmodifiers are correctly implemented to address the tooltip cutoff issue described in the linked issue. SettingpreventOverflowtoenabled: truewithboundariesElement: "viewport"ensures that tooltips do not extend beyond the viewport, which is crucial for tooltips near the screen edges. Additionally, enabling theflipmodifier allows the tooltip to reposition itself to remain visible, which is a smart use of the Popper.js library's capabilities.These changes are well-aligned with the PR objectives to improve tooltip visibility and positioning in edge cases.
78-84:Verify Integration with Existing ModifiersGiven the integration of new modifiers with existing ones (
...props.modifiers), it's critical to ensure that there are no conflicts or unexpected behaviors when these modifiers are combined with others that might be passed as props.
| preventOverflow:{ | ||
| enabled:true, | ||
| boundariesElement:"viewport", | ||
| }, | ||
| flip:{ | ||
| enabled:true, | ||
| }, |
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.
Consider Adding Comments for Clarity
While the code changes are effective, adding comments explaining why these specific modifiers were chosen could be beneficial for future maintenance. This is especially important as tooltips are a common UI component and these changes might have nuanced impacts on different parts of the application.
preventOverflow: {+ // Enable to prevent the tooltip from overflowing the viewport enabled: true,+ // Set the boundary element to the viewport to ensure visibility boundariesElement: "viewport", }, flip: {+ // Enable flipping to adjust tooltip position dynamically enabled: true, },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.
| preventOverflow:{ | |
| enabled:true, | |
| boundariesElement:"viewport", | |
| }, | |
| flip:{ | |
| enabled:true, | |
| }, | |
| preventOverflow:{ | |
| // Enable to prevent the tooltip from overflowing the viewport | |
| enabled:true, | |
| // Set the boundary element to the viewport to ensure visibility | |
| boundariesElement:"viewport", | |
| }, | |
| flip:{ | |
| // Enable flipping to adjust tooltip position dynamically | |
| enabled:true, | |
| }, |
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
This PR has been closed because of inactivity. |
leuedaniel commentedDec 18, 2024
@Nikhil-Nandagopal is there a chance to merge this request? |
jacquesikot commentedFeb 12, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@admirhusic your changes look good, can you please ad cypress tests for the change? ThisPR can help |
Uh oh!
There was an error while loading.Please reload this page.
Description
Fixes tooltip on input label when touching edges of the screen.
Fixes#34445
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Nikhil-Nandagopal I haven't created tests since this is a small change. Let me know if this case is required to be tested. :)
Summary by CodeRabbit