fix: tooltip position on edge of screen #34518
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#flip Resolves: appsmithorg#34445
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 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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 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.
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. |
|
@Nikhil-Nandagopal is there a chance to merge this request? |
jacquesikot
changed the title
|
@admirhusic your changes look good, can you please ad cypress tests for the change? This PR can help |
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