Skip to content
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

Closed
wants to merge 1 commit into from
Closed

fixed #1017

wants to merge 1 commit into from

Conversation

msubham193
Copy link

@msubham193 msubham193 commented Apr 12, 2023

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:

  • The commit message follows the guidelines in CONTRIBUTING.md.
  • Documentation (README.md) and examples have been updated as needed.
  • If this is a code change, a spec testing the functionality has been added.
  • If the commit message has [changed] or [removed], there is an upgrade path above.

@ErikDeviashin
Copy link

ErikDeviashin commented Apr 13, 2023

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
Copy link

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

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.

@diasbruno
Copy link
Collaborator

@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
Copy link
Collaborator

Let me know if you need anything, @msubham193.

@diasbruno
Copy link
Collaborator

@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
Copy link

@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
Copy link
Collaborator

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

@diasbruno diasbruno closed this Apr 20, 2023
@diasbruno
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants