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

[Modal] Dimmer-Click closed Modal when allowMultiple=true even if onHide()=false #288

Merged

Conversation

lubber-de
Copy link
Member

@lubber-de lubber-de commented Dec 4, 2018

Description

Trying to close a modal by clicking in the dimmer area but having return false at the onHide-Method did still close the modal if allowMultiple was set to true

Testcase

Unfixed

https://jsfiddle.net/ibelar/h8a3se4o/

Fixed

https://jsfiddle.net/bagwd05n/

Closes

#215
#284

@lubber-de lubber-de added type/bug Any issue which is a bug or PR which fixes a bug lang/javascript Anything involving JavaScript state/awaiting-reviews Pull requests which are waiting for reviews labels Dec 4, 2018
@prudho
Copy link
Contributor

prudho commented Dec 5, 2018

Modal event onHide seem to be triggered multiple times the second time you open the modal.

@lubber-de
Copy link
Member Author

You mean when clicking on the button within the first modal after opening the first (just because the fiddle does not allow to close any modal when it is openened once) ? Then it's wanted behavior because the onhide is called for each opened modal when clicking on the dimmer. The fiddle defines the onhide method globally in the modals prototype so it is assigned to every modal

@prudho
Copy link
Contributor

prudho commented Dec 5, 2018

I have set return true in the onHide callback.

@lubber-de
Copy link
Member Author

😱 You are right, something is wrong with the attached events... i'll take care of it. Thanks for finding it out

@y0hami y0hami added the state/on-hold Issues and pull requests which are on hold for any reason label Dec 5, 2018
@lubber-de lubber-de self-assigned this Dec 5, 2018
@lubber-de
Copy link
Member Author

lubber-de commented Dec 5, 2018

@prudho I fixed it now, was quite nasty. It was indeed totally bugged when using multiple modals, because the return false of onHide was not respected within the loop in hideAll ... Anyway, please review again, the jsfiddle from above should have been automatically updated by the included link to modal.js the latest repo-branch.
Please note: onHide (and so the alert) will be triggered for each open modal. So:

  • onHide returns true and 2 modals are open -> 2 alerts will be shown
  • onHide returns false and 2 modals are open -> only 1 alert is shown
  • onHide returns true or false and 1 modal is open -> only 1 alert is shown

@lubber-de lubber-de removed the state/on-hold Issues and pull requests which are on hold for any reason label Dec 5, 2018
@lubber-de
Copy link
Member Author

lubber-de commented Dec 5, 2018

🙄 Also fixed dimmer-click now when allowMultiple=false and onHide return false...

Copy link
Contributor

@prudho prudho left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@y0hami y0hami left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/javascript Anything involving JavaScript type/bug Any issue which is a bug or PR which fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants