-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 #7220: resolve global esc key listener conflict in Calendar component #7228
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
components/lib/calendar/Calendar.js
Outdated
@@ -1852,7 +1854,7 @@ export const Calendar = React.memo( | |||
setOverlayVisibleState(true); | |||
|
|||
overlayEventListener.current = (e) => { | |||
if (!isOutsideClicked(e)) { | |||
if (!isOutsideClicked(e.target)) { |
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 line looks incorrect as isOutsideClicked
takes an event
and gets its event.target
?
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.
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.
OK but if you look at the code
const isOutsideClicked = (event) => {
return elementRef.current && !(elementRef.current.isSameNode(event.target) || isNavIconClicked(event.target) || elementRef.current.contains(event.target) || (overlayRef.current && overlayRef.current.contains(event.target)));
};
That means if you pass e.target
then inside isOutSideClicked they will be calling target.target
does that make sense? the Target is a DOM element and it won't have a target property?
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.
Thanks for the clarification, I missed that detail. I'll remove the target and push the update. Can I address the other issue in a different PR?
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.
you can keep modifying this PR its OK.
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.
noticed missing condition, added a fix to resolve the issue.
I am using
Maybe better to create a new task, right? |
I've fixed this issue, but it hasn't been pushed to production yet. You can track further details here. #7220 @mrpsiho |
Fix #7220
This also resolves the following issues: