-
Notifications
You must be signed in to change notification settings - Fork 6.7k
fix(modal): fix for conflicts with ngTouch module on mobile devices #3044
Conversation
Thank you for this solution! This solved my problem. When is it going to be merged?? |
fix(modal): fix for conflicts with ngTouch module on mobile devices
Hi guys, sorry, for such a noob question: Thanks |
Not that I'm aware of sorry (I'm new here myself) |
@chrisirhc - This was the other PR I merged without cherry picking - shall I revert and add a REady to Merge label? |
Has this change been reverted again? When I download the latest version (0.13.0), looking in the template code for modals, I find This is still causing issues when ngTouch is enabled. |
Yes, the merge was reverted by @chrisirhc. Why? |
Sorry, I reverted it initially because it didn't follow our merging guidelines. And I believe we forgot about re-applying the change since then. Reopening #2280 to avoid confusion. |
Looking forward to this fix being re-merged at some point... |
👍 |
+1 Please ! |
|
Hi, Is there any update when this change can be merged? Is there any work around solution to fix this issue? Thanks, |
The fix is in one of the links above. This is what I did.. in ui-boostrap-tpls.js /* THIS WAS THE ORIGINAL Then in ui-boostrap.js added the element.on function .directive('modalWindow', [
|
@chrisirhc Anything specific that was wrong in the PR? Would love to resubmit PR to have this implemented. Could you point at the right direction what was wrong with the PR? |
@mastef, I don't think there'll be any issues, but given the age of this, please rebase and ensure your code is fully up-to-date and that this PR would still have no conflicts. additionally, we've since implemented a CI build that will get kicked off when you submit a PR that was clearly instituted after you submitted this originally. I don't know that we'll require it given the nature of this change, but it might be nice just for sanity sake. @wesleycho, would you have any hesitations merging this as is or if you had other stipulations, what would they be? |
I don't think there were any issues (besides lack of tests, but I'm not sure how to test, and that shouldn't block it), it just needs to be re-cherry-picked into the code. |
I'm still a bit baffled why this fixes the issue, but I fon't have any particular objections if it works |
I believe it's because ngTouch adds an ngClick directive that attempts to be smart about detecting where you're tapping. However, in this case, it incorrectly detects tapping on the modal while it should instead tap into elements within the modal. |
- Merge pull request angular-ui#3044 from jiniguez/fix2280 Conflicts: src/modal/modal.js template/modal/window.html Fixes angular-ui#2280
- Merge pull request angular-ui#3044 from jiniguez/fix2280 Conflicts: src/modal/modal.js template/modal/window.html Fixes angular-ui#2280
- Merge pull request angular-ui#3044 from jiniguez/fix2280 Conflicts: src/modal/modal.js template/modal/window.html Fixes angular-ui#2280 Closes angular-ui#4357
This change attempts to fix issue #2280
ng-click directive has been removed from modal template and click event moved to the modal-window directive code, so the click event keeps firing and modal window closes when clicking on the backdrop. Not sure if there's a better place to put this event but the fix seems to be working.