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

fix(modal): change the mouse event to dismiss a modal on backdrop click #5326

Merged

Conversation

florenthobein
Copy link
Contributor

Because of the use of a 'click' event to trigger the dismissal of a modal, the target of the event doesn't take into account an eventual movement of the mouse before releasing click (for ex. selection of text), which can lead to unwanted dismissal of the modal.

Closes: #5264

PR Checklist

  • read and followed the CONTRIBUTING.md guide.
  • built and tested the changes locally.
  • added/updated tests.
  • added/updated API documentation (not needed)
  • added/updated demos (not needed)

Because of the use of a 'click' event to trigger the dismissal of a modal, the target of the event doesn't take into account an eventual movement of the mouse before releasing click (for ex. selection of text), which can lead to unwanted dismissal of the modal.

Closes: valor-software#5264
@codecov
Copy link

codecov bot commented Jul 12, 2019

Codecov Report

Merging #5326 into development will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #5326   +/-   ##
============================================
  Coverage        51.54%   51.54%           
============================================
  Files                3        3           
  Lines               97       97           
  Branches            17       17           
============================================
  Hits                50       50           
  Misses              37       37           
  Partials            10       10

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efa5229...4dfc475. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 12, 2019

Codecov Report

Merging #5326 into development will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #5326   +/-   ##
============================================
  Coverage        52.52%   52.52%           
============================================
  Files                3        3           
  Lines               99       99           
  Branches            17       17           
============================================
  Hits                52       52           
  Misses              37       37           
  Partials            10       10

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a134be4...4a1af09. Read the comment docs.

Copy link

@benjaminrau benjaminrau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix which is bugging us also. I tried to provide a review.

src/modal/modal-container.component.ts Outdated Show resolved Hide resolved
src/modal/modal-container.component.ts Outdated Show resolved Hide resolved
src/modal/modal-container.component.ts Outdated Show resolved Hide resolved
src/modal/modal-container.component.ts Show resolved Hide resolved
@benjaminrau
Copy link

@florenthobein We tested your changes in our project and it didnt fix the issue in first place.
We use ModalDirective instead of the Component. After we applied your changes on the directive it is fixed in both.

I think the Directive should be fixed within this pull request also.

korinovsky
korinovsky previously approved these changes Sep 10, 2019
@Lexikus
Copy link

Lexikus commented Oct 15, 2019

How is this task going? It might fix a problem that I have.

Because of the use of a 'click' event to trigger the dismissal of a modal, the target of the event doesn't take into account an eventual movement of the mouse before releasing click (for ex. selection of text), which can lead to unwanted dismissal of the modal.
@florenthobein
Copy link
Contributor Author

florenthobein commented Oct 17, 2019

I just pushed an updated version taking in consideration comments about variable naming & including this fix in the context of a directive.

Sadly enough, there is no shared code between the ModalDirective and the ModalContainerComponent. I didn't take the risk to initiate a refactoring of this component, and had to duplicate part of the logic.

Tests are OK. Any update on a potential merge? 🙏

@daniloff200
Copy link
Contributor

Sorry for the long delay, I'm gonna review it and share with the QA team very soon

@daniloff200 daniloff200 self-assigned this Jan 13, 2020
@florenthobein
Copy link
Contributor Author

There is something I would like to get your attention on, I have a strange behavior on my app using this patch and I'm not quite sure it's linked to this PR yet. When trying to scroll the modal using the mouse on scrollbar (drag&drop of the scrollbar), it closes the modal (verified on Chrome on a Macbook without mouse plugged). Do you reproduce this same behavior?

Copy link
Member

@valorkin valorkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for contribution, looks good to me

@daniloff200
Copy link
Contributor

@florenthobein well, I hope @dmitry-zhemchugov will help us to find out, is it an issue, related to this PR, or, something new, or, some special use case, which you found suddenly!

@daniloff200 daniloff200 merged commit 74f752f into valor-software:development Jan 20, 2020
@florenthobein florenthobein deleted the modal-dismiss-event branch May 20, 2020 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modal closes on release of mouse outside modal box
8 participants