reactjs / react-modal

Accessible modal dialog component for React
http://reactcommunity.org/react-modal
MIT License
7.37k stars 808 forks source link

fixed #1017

Closed msubham193 closed 1 year ago

msubham193 commented 1 year ago

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:

ErikDeviashin commented 1 year ago

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.

ErikDeviashin commented 1 year ago

Also will be good to fill Reviewers, Assignees and, if there is some labels in repo, Labels fields.

diasbruno commented 1 year ago

@msubham193 as @ErikDeviashin pointed out, there are a few things that needs to be clean up,

Also will be good to fill Reviewers, Assignees and, if there is some labels in repo, Labels fields.

No need for this. It's just a simple patch.

To update the commit message, use git commit --amend -m "new message". To remove the package-lock.json, use git checkout -- package-lock.json && git commit --amend

If you run this command in sequence, it will do the right thing (hopefully).

diasbruno commented 1 year ago

Let me know if you need anything, @msubham193.

diasbruno commented 1 year ago

@ErikDeviashin Can you please help providing a test case for this patch? I think this is going to be the hardest part of this patch.

ErikDeviashin commented 1 year ago

@diasbruno No, it wasn't that hard, to be honest. Especially considering the many other test case examples available. Can you provide me access to push in the repository so I can push the changes? Or how would you like it to be done? Also, I found that two other test cases (about mouse down on content/overlay and mouse up on the opposite element) weren't really working, so I fixed them too.

diasbruno commented 1 year ago

As you are interested in this fix, @ErikDeviashin, you can take over...

diasbruno commented 1 year ago

@msubham193 Thanks so much for helping with this. We are going to move on due to some changes that this PR needs.