Skip to content

Comments

Fix: Link dialog opens outside screen boundaries in checkbox question…#24461

Closed
Hetsavani wants to merge 1 commit intocalcom:mainfrom
Hetsavani:Fixes-Link-dialog-opens-outside-screen-boundaries
Closed

Fix: Link dialog opens outside screen boundaries in checkbox question…#24461
Hetsavani wants to merge 1 commit intocalcom:mainfrom
Hetsavani:Fixes-Link-dialog-opens-outside-screen-boundaries

Conversation

@Hetsavani
Copy link

… editor

It fixes the UI issue of the toggle popup going outside the screen while using smartphone.

I have changed some stylings so that the whole popup appears on the screen.

Screencast.from.2025-10-15.00-04-42.webm

Mandatory Tasks (DO NOT REMOVE)

  • [✅] I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • [✅] I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • [✅] I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

Follow the steps to reproduce in the issue.

@Hetsavani Hetsavani requested review from a team as code owners October 14, 2025 18:39
@vercel
Copy link

vercel bot commented Oct 14, 2025

@Hetsavani is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@graphite-app graphite-app bot added the community Created by Linear-GitHub Sync label Oct 14, 2025
@github-actions github-actions bot added ui area: UI, frontend, button, form, input 🐛 bug Something isn't working labels Oct 14, 2025
@github-actions
Copy link
Contributor

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

Unknown release type "Fix" found in pull request title "Fix: Link dialog opens outside screen boundaries in checkbox question…". 

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 14, 2025

Walkthrough

Updated the editor popup positioning logic to be viewport-aware. Instead of assigning top/left directly from element rects, the code now calculates positions using window dimensions, applies horizontal and vertical bounds with padding, and repositions the popup above the trigger if it would overflow below. The calculations ensure the popup stays within screen boundaries on smaller viewports (e.g., mobile/tablet). No exported or public API changes were made.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly describes the main change of fixing the link dialog opening outside the screen boundaries for checkbox questions, directly reflecting the issue addressed by the code.
Linked Issues Check ✅ Passed The updated positioning logic constrains the popup within viewport bounds with padding and flips it above the trigger when needed, fulfilling the requirements of issues #24267 and CAL-6520 by ensuring the dialog remains fully visible and responsive on mobile and tablet devices.
Out of Scope Changes Check ✅ Passed All code modifications are limited to the toolbar plugin’s popup positioning logic and directly address the linked bug reports, with no unrelated changes detected in the pull request.
Description Check ✅ Passed The description clearly outlines the UI issue being fixed, references the relevant issues, and summarizes the styling changes to ensure the popup appears within the viewport, matching the changes in the code.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@dosubot dosubot bot added the 🧹 Improvements Improvements to existing features. Mostly UX/UI label Oct 14, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (1)
packages/ui/components/editor/plugins/ToolbarPlugin.tsx (1)

64-74: Consider extracting magic numbers as constants.

The padding values (10px) are hardcoded in multiple places. Extracting them as named constants would improve maintainability and make it easier to adjust padding consistently if needed.

Add constants at the module level:

+const POPUP_VIEWPORT_PADDING = 10;
+
 function positionEditorElement(editor: HTMLInputElement, rect: DOMRect | null) {
   if (rect === null) {
     editor.style.opacity = "0";
     editor.style.top = "-1000px";
     editor.style.left = "-1000px";
   } else {
     const viewportWidth = window.innerWidth;
     const viewportHeight = window.innerHeight;
 
-    let top = rect.top + rect.height + window.pageYOffset + 10;
+    let top = rect.top + rect.height + window.pageYOffset + POPUP_VIEWPORT_PADDING;
     let left = rect.left + window.pageXOffset - editor.offsetWidth / 2 + rect.width / 2;
 
     // Ensure the popup doesn't go outside the viewport horizontally
     if (left < 0) {
-      left = 10; // Add some padding from the left edge
+      left = POPUP_VIEWPORT_PADDING;
     } else if (left + editor.offsetWidth > viewportWidth) {
-      left = viewportWidth - editor.offsetWidth - 10; // Add some padding from the right edge
+      left = viewportWidth - editor.offsetWidth - POPUP_VIEWPORT_PADDING;
     }
 
     // Ensure the popup doesn't go outside the viewport vertically
     if (top + editor.offsetHeight > viewportHeight + window.pageYOffset) {
-      top = rect.top + window.pageYOffset - editor.offsetHeight - 10; // Position above the element
+      top = rect.top + window.pageYOffset - editor.offsetHeight - POPUP_VIEWPORT_PADDING;
     }
 
     editor.style.opacity = "1";
     editor.style.top = `${top}px`;
     editor.style.left = `${left}px`;
   }
 }
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b5585b3 and 0bdb2bb.

📒 Files selected for processing (1)
  • packages/ui/components/editor/plugins/ToolbarPlugin.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

Always use t() for text localization in frontend code; direct text embedding should trigger a warning

Files:

  • packages/ui/components/editor/plugins/ToolbarPlugin.tsx
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/review.mdc)

Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js .utc() in hot paths like loops

Files:

  • packages/ui/components/editor/plugins/ToolbarPlugin.tsx
**/*.{ts,tsx,js,jsx}

⚙️ CodeRabbit configuration file

Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.

Files:

  • packages/ui/components/editor/plugins/ToolbarPlugin.tsx
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (1)
packages/ui/components/editor/plugins/ToolbarPlugin.tsx (1)

58-79: Clamp popup top to viewport boundary

After repositioning above the trigger (line 73), ensure top is at least window.pageYOffset + 10 to prevent negative values when the element is near the viewport top. Also review other direct style.top/left assignments in packages/features/ee/support/components/IntercomContactForm.tsx and packages/app-store/hitpay/components/HitPayDropIn.ts and add similar viewport constraints if needed.

@dhairyashiil
Copy link
Member

Hi @Hetsavani,

Thank you for taking the time to work on this issue and submit a PR.
After review, we've decided to move forward with PR #24268 instead.

The differences are relatively minor refinements, but the most robust solution is merged.

We encourage you to keep contributing to Cal.com
Here are some suggestions:
Check out other open issues that need attention
Feel free to reach out if you need help with anything.

@Hetsavani
Copy link
Author

Hi @Hetsavani,

Thank you for taking the time to work on this issue and submit a PR. After review, we've decided to move forward with PR #24268 instead.

The differences are relatively minor refinements, but the most robust solution is merged.

We encourage you to keep contributing to Cal.com Here are some suggestions: Check out other open issues that need attention Feel free to reach out if you need help with anything.

Thank you so much!
I will be grateful if you provide details of what’s wrong in my PR and what was there in the other PR 😊
As it will be helpful in my future contributions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐛 bug Something isn't working community Created by Linear-GitHub Sync 🧹 Improvements Improvements to existing features. Mostly UX/UI size/S ui area: UI, frontend, button, form, input

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Link dialog opens outside screen boundaries in checkbox question editor

3 participants