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 option destroyOnHide #72

Merged
merged 3 commits into from
Dec 28, 2017
Merged

Conversation

Rohanhacker
Copy link
Contributor

@Rohanhacker Rohanhacker commented Dec 15, 2017

Adding destroyOnHide to Dialog unmount child components

Addresses #22 && Add [destroyOnHide] to Modal #8494

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 75.926% when pulling 9cfe5b1 on Rohanhacker:destroyonHide into 6546b60 on react-component:master.

src/Dialog.tsx Outdated
>
{props.children}
let dialogElement;
if (props.visible || !props.destroyOnHide) {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delayed response. But I think here is the better place to add this conditional statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case leaving animation won't work because we are using <Animate inside Dialog and if we unmount its parent then leaving animation won't work @yesmeck

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, you are right, how about put it to the below {(props.visible || !props.destroyOnHide) && dialogElement}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sense will update the pr @yesmeck

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, could you write a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yesmeck done

@coveralls
Copy link

Coverage Status

Coverage remained the same at 75.701% when pulling 4ffd5dd on Rohanhacker:destroyonHide into 6546b60 on react-component:master.

@yesmeck yesmeck merged commit aa73a36 into react-component:master Dec 28, 2017
@yesmeck
Copy link
Member

yesmeck commented Dec 28, 2017

@Rohanhacker Great work, I renamed destroyOnHide to destroyOnClose, because all other props call "hide" as "close" link onCloseafterClose. And I've release 7.1.0

@yesmeck
Copy link
Member

yesmeck commented Dec 28, 2017

@Rohanhacker Could you upgrade antd's rc-dialog to 7.1.0 and update the document?

@Rohanhacker
Copy link
Contributor Author

yup will send a pr

@Rohanhacker
Copy link
Contributor Author

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

3 participants