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

Add modal fade option #287

Merged
merged 2 commits into from May 28, 2019
Merged

Add modal fade option #287

merged 2 commits into from May 28, 2019

Conversation

schorke
Copy link

@schorke schorke commented May 20, 2019

Added the fade option in modal, default is set to true to not break the current behaviour.

@thednp
Copy link
Owner

thednp commented May 20, 2019

You want to add true/false to the class?

@schorke
Copy link
Author

schorke commented May 21, 2019

I wanted to add the ability to remove the "fade" class on the <div class="modal-backdrop fade"></div> by changing newOverlay[setAttribute]('class',modalBackdropString+' fade'); to newOverlay[setAttribute]('class', modalBackdropString + (self[animation] ? ' fade' : ''));.

@thednp
Copy link
Owner

thednp commented May 28, 2019

@schorke I think an "auto" mode would suffice much better. Think about this:

if (hasClass(modal,'fade')) // also add fade class to the backdrop

We don't change the original plugin options while adding a simple fail safe mechanism, all without developer having to remember "one more thing".

What do you think?

@schorke
Copy link
Author

schorke commented May 28, 2019

I don't understand the "auto mode". How / where do you set or remove the fade ?

@thednp
Copy link
Owner

thednp commented May 28, 2019

Basically we change this

this[animation] = options[animation] === false || modal[getAttribute](animation) === 'false' ? false : true;

to this

this[animation] = hasClass(modal,'fade') ? true : false;

@schorke what do you think?

@schorke
Copy link
Author

schorke commented May 28, 2019

Ok, now I understand !

Yeah, I think your solution is better : keeping it out of options and managed by class ! I'll update my PR. 👍

@thednp thednp merged commit d1e6519 into thednp:master May 28, 2019
@thednp
Copy link
Owner

thednp commented May 28, 2019

This will require additional tinkering for the case when opening another modal from a currently opened modal. The first example of our demo shows that the <div id="anotherModal> with/without the fade class will hide the backdrop.

thednp added a commit that referenced this pull request May 28, 2019
thednp added a commit that referenced this pull request May 28, 2019
@thednp
Copy link
Owner

thednp commented May 28, 2019

Done. Please download latest master and give it a try, I'm interested in the modal original events and opening modal from another modal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants