-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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: Peek overlay overflowing in side by side #33376
Conversation
WalkthroughWalkthroughThe update introduces a function Changes
Assessment against linked issues
The changes align well with the objective of ensuring the peak overlay is fully visible in split view scenarios, addressing the issue comprehensively. Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
/build-deploy-preview skip-test=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9059961163. |
Deploy-Preview-URL: https://ce-33376.dp.appsmith.com |
/build-deploy-preview |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9061053695. |
Deploy-Preview-URL: https://ce-33376.dp.appsmith.com |
@@ -96,6 +96,11 @@ export function PeekOverlayPopUpContent( | |||
PEEK_OVERLAY_DELAY, | |||
); | |||
|
|||
const getLeftPosition = (position: DOMRect) => { |
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.
Why are we not using the Popover here and its ability to avoid / contain within certain HTML elements. The absolute positioning feels like we would need to handle it whenever dimensions change
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.
This change requires a design overlook cause popover comes with it's on padding shadow and all... Will create a task and assign to Design team.
Description
Peek overlay popover was getting cut off in the left side. This PR fix this issue.
Fixes #33370
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9059955985
Commit: a7a9a10
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?