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): don't close popup on ESC when closing nested components #3384

Merged

Conversation

maxokorokov
Copy link
Member

@maxokorokov maxokorokov commented Sep 27, 2019

Fixes an issue for datepicker, typeahead, dropdown inside the modal.
When these components are closed on ESC, modal stays open.

Nested

The idea is to:

  • use the same keyup for all ESC autoclose handling
  • make modal keyup check run after all other autoclose checks
  • don't close the modal if event was default prevented

Fixes #3358

Fixes an issue for datepicker, typeahead, dropdown inside the modal.
When these components are closed on ESC, modal stays open.

Fixes ng-bootstrap#3358
@codecov-io
Copy link

codecov-io commented Sep 27, 2019

Codecov Report

Merging #3384 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3384      +/-   ##
==========================================
+ Coverage   90.86%   90.87%   +0.01%     
==========================================
  Files          95       95              
  Lines        2748     2752       +4     
  Branches      511      511              
==========================================
+ Hits         2497     2501       +4     
  Misses        190      190              
  Partials       61       61
Flag Coverage Δ
#e2e 55.15% <100%> (+0.1%) ⬆️
#unit 87.92% <81.81%> (-0.02%) ⬇️
Impacted Files Coverage Δ
src/util/autoclose.ts 84% <100%> (ø) ⬆️
src/modal/modal-window.ts 96.87% <100%> (+0.44%) ⬆️

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 d8812b9...1b7dae5. Read the comment docs.

@fbasso
Copy link
Member

fbasso commented Sep 27, 2019

Thanks @maxokorokov

I fail to find a simpler solution for this, so LGTM :)

@maxokorokov
Copy link
Member Author

@fbasso thanks, merging!

@maxokorokov maxokorokov merged commit 75f0966 into ng-bootstrap:master Sep 27, 2019
maxokorokov added a commit to maxokorokov/ng-bootstrap that referenced this pull request Nov 8, 2019
This uses a combination of 'mousedown/up' events instead of 'click'.
This aligns behavior with bootstrap and helps with closing modal accidentally (ex. when selecting text)

Some tests had to be converted to e2e

Closes ng-bootstrap#3384
Fixes ng-bootstrap#1950
maxokorokov added a commit that referenced this pull request Nov 8, 2019
This uses a combination of 'mousedown/up' events instead of 'click'.
This aligns behavior with bootstrap and helps with closing modal accidentally (ex. when selecting text)

Some tests had to be converted to e2e

Closes #3384
Fixes #1950
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.

Closing modal with ESC is interrupted by datepicker
3 participants