Skip to content

Commit

Permalink
Merge pull request #208 from alexwhin/master
Browse files Browse the repository at this point in the history 
Fix #198 pressing enter key on modal
  • Loading branch information
@Emkae
Emkae committed Jul 17, 2017
2 parents 706cb74 + f16ca25 commit 78434a6
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions jquery.modal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
modals.push(this);
if (el.is('a')) {
target = el.attr('href');
this.anchor = el;
//Select element by id from href
if (/^#/.test(target)) {
this.$elm = $(target);
Expand Down Expand Up @@ -83,6 +84,7 @@
open: function() {
var m = this;
this.block();
this.anchor.blur();
if(this.options.doFade) {
setTimeout(function() {
m.show();
Expand Down

3 comments on commit 78434a6

@gorillamoe
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can revert, if @kylefox doesn't like this approach. But I like this approach. It has no ill side effects and the naming is also good. anchor is correct when using a tags, but also works for every other tag that fires up a jquery modal. Seems pretty good to me 👍

@kylefox
Copy link
Owner

Choose a reason for hiding this comment

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

There is a problem with this approach: not all modal are opened via an <a> tag, i.e. when manually opening a modal. In this case, this.anchor is undefined and an error is thrown: TypeError: Cannot read property 'blur' of undefined

Have a look at the second example in Example 4 — you'll see this error logged to the console.

To complicate things further (😂) I don't think we can fix by simply wrapping this.anchor.blur(); in a conditional, like this:

if(this.anchor && this.anchor.blur) {
  this.anchor.blur();
}

because we're back to square one — hitting enter re-triggers the open event (albeit for manually-opened modals only).

I think we may need a solution that is independent of links/clicking and instead prevents a modal from being re-opened if that modal is already open. But I'm not entirely sure how to do that, would need to think about it more.

Either that, or we simply use the wrapped approach above, and require users to solve the problem for manually-opened modals themselves. However, I do think it should just work out of the box.

Any ideas?

@gorillamoe
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, snap!

I reverted the master branch back to the last working commit which is 706cb74.
I also added a workaround-keypress-enter branch, which is now 2 commits behind the current branch (because I reverted the master branch). Once we got the new (temporary) branch working, and 6 eyes didn't catch any ill side effects, we merge it back to master (if @kylefox signs it off). I will try to fix it this evening or tomorrow morning (GMT +0200).

Please sign in to comment.