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] Add TransitionHandlers to Modal props TypeScript definitions #9723

Merged
merged 2 commits into from
Jan 4, 2018
Merged

[Modal] Add TransitionHandlers to Modal props TypeScript definitions #9723

merged 2 commits into from
Jan 4, 2018

Conversation

pvdstel
Copy link
Contributor

@pvdstel pvdstel commented Jan 3, 2018

Resolves #9689.

As of beta 26, it is no longer possible to use transition event handlers (onEnter, onExit, and their variations) in several components using the Modal base component. The implementation has not been removed however, only the TypeScript definitions have been modified to no longer expose these APIs.

As discussed in the issue mentioned above (specifically this comment), this change should be reverted. Thus, the TransitionHandler props have been added to ModalProps, and transitively the components extending ModalProps.

@pvdstel pvdstel changed the title Add TransitionHandlers to Modal props TS definitions Add TransitionHandlers to Modal props TypeScript definitions Jan 3, 2018

export interface ModalProps
extends StandardProps<
React.HtmlHTMLAttributes<HTMLDivElement> & Partial<PortalProps>,
React.HtmlHTMLAttributes<HTMLDivElement> & Partial<PortalProps> & Partial<TransitionHandlers>,
Copy link
Member

Choose a reason for hiding this comment

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

The properties aren't implemented on the component. Should we move it to the dependents (Drawer, Dialog, Snackbar, Popover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The properties used to be in ModalProps, but if the actual implementation is in the dependents I'd say it's more appropriate to move them there. Should I?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some dependents (Popover, Snackbar) still had the TransitionHandlers props so I left those alone. Also, the Snackbar props do not expose the Modal props, is that by design?

@oliviertassinari oliviertassinari changed the title Add TransitionHandlers to Modal props TypeScript definitions [Modal] Add TransitionHandlers to Modal props TypeScript definitions Jan 3, 2018
@pvdstel
Copy link
Contributor Author

pvdstel commented Jan 3, 2018

Not entirely sure why the CI is failing but I'm fairly sure it's not because of the definition changes.

@oliviertassinari
Copy link
Member

Maybe you can try to rebase on top of v1-beta and push force.

Some dependents still had the TransitionHandlers props, these have not been modified.
@pvdstel
Copy link
Contributor Author

pvdstel commented Jan 4, 2018

It failed the first time, second time it worked... weird.

@oliviertassinari oliviertassinari merged commit 6a1a648 into mui:v1-beta Jan 4, 2018
@oliviertassinari
Copy link
Member

@pvdstel Thanks!

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

Successfully merging this pull request may close these issues.

2 participants