-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: DatePicker in Modal requires two clicks to open calendar #21109
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21109 +/- ##
==========================================
- Coverage 92.60% 92.59% -0.02%
==========================================
Files 515 515
Lines 38225 38234 +9
Branches 5863 5839 -24
==========================================
+ Hits 35397 35401 +4
- Misses 2678 2683 +5
Partials 150 150
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
adamalston
left a comment
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.
Can you add testing?
| // Auto-detect if DatePicker is in a modal and use appendTo for proper positioning | ||
| let effectiveAppendTo = appendTo; | ||
| if (!effectiveAppendTo && start) { | ||
| // Check if the input is inside a modal | ||
| const modalElement = start.closest(`.${prefix}--modal`); | ||
| if (modalElement) { | ||
| // Find the modal container to append the calendar to | ||
| const modalContainer = modalElement.querySelector( | ||
| `.${prefix}--modal-container` | ||
| ); | ||
| if (modalContainer) { | ||
| effectiveAppendTo = modalContainer as HTMLElement; | ||
| } | ||
| } | ||
| } |
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.
I have a few concerns:
- This change appears to target only modals. Are you certain the issue is limited to modals?
- This functionality relies on class selectors. Is there a more robust way to target modals than using class names? I have the same concern regarding the changes in
Modal.tsx.
fix: #21092
Fix: Auto-detect modals and append calendar to modal container + prevent Modal's auto-scroll from interfering with DatePicker positioning.
Result: Calendar opens correctly on first click in scrollable modals.