-
Notifications
You must be signed in to change notification settings - Fork 808
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
fixed #1017
fixed #1017
Conversation
Hi @msubham193! Please read contributing guidelines, look at examples of others, already merged PR, and rewrite PR subject and description accordingly. Rewrite the description wisely, describing what problem you solving, not how - it can be seen in files changes anyway. |
Also will be good to fill Reviewers, Assignees and, if there is some labels in repo, Labels fields. |
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 file shouldn't appear in this PR. With it you change many locked versions of lidraries, which affects many dependencies throughout project and can lead to version incompatibilities, unpredictable behavior, and/or deployment issues. Changes of this kind should only be made with the approval of the maintainers and when you really know what and why you are changing. You can read little more about package-lock.json file on npm docs site. Please remove it from the PR.
@msubham193 as @ErikDeviashin pointed out, there are a few things that needs to be clean up,
No need for this. It's just a simple patch. To update the commit message, use If you run this command in sequence, it will do the right thing (hopefully). |
Let me know if you need anything, @msubham193. |
@ErikDeviashin Can you please help providing a test case for this patch? |
@diasbruno |
As you are interested in this fix, @ErikDeviashin, you can take over... |
@msubham193 Thanks so much for helping with this. We are going to move on due to some changes that this PR needs. |
Fixes #1015.
Changes proposed:
added this.shouldClose = null; to the handleOverlayOnMouseDown method (src/components/ModalPortal.js:321) this allow the modal to close in cases where the mouse press started on the overlay. Then, added the event.stopPropagation(); handleContentOnMouseDown method (src/components/ModalPortal.js:331)
Upgrade Path (for changed or removed APIs):
Acceptance Checklist:
CONTRIBUTING.md
.