🐛 Fix #6252: Fix the toggle calendar utility to properly close the datepicker#6253
Open
balajis-qb wants to merge 1 commit intoHacker0x01:mainfrom
Open
🐛 Fix #6252: Fix the toggle calendar utility to properly close the datepicker#6253balajis-qb wants to merge 1 commit intoHacker0x01:mainfrom
balajis-qb wants to merge 1 commit intoHacker0x01:mainfrom
Conversation
- Update click outside detection to use closest method for class matching. - Modified the click outside detection logic to utilize the `closest` method for checking ignored elements, ensuring compatibility with Element targets. - Updated the corresponding test case for clarity. Closes Hacker0x01#6252
55e5769 to
699278f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6253 +/- ##
==========================================
+ Coverage 99.29% 99.31% +0.02%
==========================================
Files 30 30
Lines 3822 3822
Branches 1648 1665 +17
==========================================
+ Hits 3795 3796 +1
+ Misses 26 25 -1
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Linked issue: #6252
Problem
As I mentioned in the attached ticket, we have an issue in toggling the calendar using
toggleCalendarOnIconClick. The issue is with theClickOutsideWrapperHOC. There we're listening globally for themousedownto detect the clicks happening outside the DatePicker to auto-close the Datepicker. As we used the Browser API directly it's getting executed first and auto-close the datepicker. Later React delegates the synthetic event to the click handler of the CalendarIcon, as we wrote a logic to toggle the open state, it's reopening the datepicker.Here the issue is with the
ClickOutsideWrapperHOC. There we're checking whether the clicked target is an instance of HTMLElement, but the calendar icon is svg won't satisfy the condition. As a result we're treating the click happening on the SVG icon as a outside click and closing the datepicker.Changes
target.classList.contains(ignoreClass)will fail as we only added theignoreClassto the parent svg and not to its child elements. So I updated the condition totarget.closest(.${ignoreClass})- as a result event it will consider the parent svg'signoreClassClickOutsideWrapperalready covers all the cases, I didn't add any extra test cases, but just updated the description of a test.Note: This utility has a few accessibility related issues. I'll work on that in my upcoming PRs.
Contribution checklist