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.

Sign up for GitHub

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

Jump to bottom

feature: add onAfterClose config option #1005

Closed
wants to merge 3 commits into from
Closed

feature: add onAfterClose config option #1005

wants to merge 3 commits into from

Conversation

Baune8D
Copy link

@Baune8D Baune8D commented Mar 9, 2018 •
edited
Loading

As described here #1004

@Baune8D Baune8D changed the title feature: add onAfterClose config option (#1004) feature: add onAfterClose config option Mar 9, 2018
@Baune8D Baune8D mentioned this pull request Mar 9, 2018
Closed
limonte
limonte approved these changes Mar 9, 2018
View reviewed changes
Copy link
Member

@limonte limonte left a comment

Choose a reason for hiding this comment

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

LGTM!

zenflow
zenflow suggested changes Mar 10, 2018
View reviewed changes
Copy link
Member

@zenflow zenflow left a comment

Choose a reason for hiding this comment

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

Looks like onAfterClose will be called before onClose if animation is supported and enabled.

Would you mind cleaning up our code a bit and changing the onComplete argument to onClose to reflect what it actually is? This will make it clearer.

@Baune8D
Copy link
Author

Baune8D commented Mar 17, 2018

@zenflow Yeah i see your point. Have a look at my latest commit.

onClose has been moved further up in the function and made sync so it will always run before the actual disposal of the modal happens.

onAfterClose will now run async after the modal has been disposed.

I updated the tests to reflect this.

It might be a problem that onClose runs differently now, if someone is dependent on the old behaviour.

@Baune8D
Copy link
Author

Baune8D commented Mar 17, 2018

@zenflow travis-ci gives a weird error? Relevant tests seems to succeed though

@zenflow
Copy link
Member

zenflow commented Mar 17, 2018

@Baune8D Travis failure is due to #986 .. I started another build ..

@limonte
Copy link
Member

limonte commented Mar 18, 2018

@Baune8D thanks for the test! Travis build is now passing, sorry for this inconvenience.

@zenflow please do CR, both of your requests are done as I can see.

@limonte
Copy link
Member

limonte commented Mar 20, 2018 •
edited
Loading

There are Git conflicts after #1026

I decided to save you time @Baune8D and create a new PR with your commits cherry-picked: #1030

I'm closing this PR, thanks again @Baune8D 🍻 for your contribution and @zenflow for catching issues!

@limonte limonte closed this Mar 20, 2018
@limonte
Copy link
Member

limonte commented Mar 20, 2018

Released in v7.17.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zenflow zenflow zenflow requested changes

@limonte limonte limonte approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Baune8D @zenflow @limonte