eBay / nice-modal-react

A modal state manager for React.
https://ebay.github.io/nice-modal-react
MIT License
1.96k stars 110 forks source link

muiDialog Function is Outdated #65

Closed af185178 closed 1 year ago

af185178 commented 2 years ago

Hello!

All of the modals in our app are based on MUI Dialogs, but we are unable to use the muiDialog function because it's passing the deprecated onExited prop. This results in a console warning that we are passing deprecated props and should be using TransitionProps instead.

See the note here in the MUI v4 documentation, also the corresponding MUI v5 documentation here.

I think the function should be using TransitionProps like so:

export const muiDialog = (modal: NiceModalHandler): { open: boolean; onClose: () => void; TransitionProps: { onExited: () => void } } => {
  return {
    open: modal.visible,
    onClose: () => modal.hide(),
    TransitionProps: {
      onExited: () => {
        modal.resolveHide();
        !modal.keepMounted && modal.remove();
      },
    },
  };
};

Overall great package though guys thanks for your work!

supnate commented 2 years ago

Hi @af185178 , thanks for the report!

I will update docs about it. For v5, we suggest not using the helper method because there may be additional props necessary on TransitionProps by user. If we use <Dialog ...muiDialog(modal) /> then it's hard for user to understand what will happen if they put TransitionProps either before or after ...muiDialog(modal).

Or, you can use:

const muiProps = muiDialog(modal);
//...
<Dialog {...muiProps} TransitionProps={{onExited: muiProps.onExited}} />
af185178 commented 2 years ago

Hey @supnate thanks for getting back to me

That makes sense, I can see why you wouldn't want to set TransitionProps in the helper function.

If that's the case though, should onExited be removed from muiDialog? In the example you gave, I think we would still get the console warning since we're passing onExited in muiProps, which seems to be deprecated in all uses. Is there ever a case where onExited should be passed outside of TransitionProps?

supnate commented 2 years ago

hmm, maybe I can added a new helper named museDialogV5 as you mentioned, or if you can add a PR is welcome🙂

export const muiDialogV5 = (modal: NiceModalHandler): { open: boolean; onClose: () => void; TransitionProps: { onExited: () => void } } => {
  return {
    open: modal.visible,
    onClose: () => modal.hide(),
    TransitionProps: {
      onExited: () => {
        modal.resolveHide();
        !modal.keepMounted && modal.remove();
      },
    },
  };
};

The only thing for user to understand is they need construct full TransitionProps after the helper method if necessary. It seems to be convenient since I think most cases of MuiDialog usage don't customize TransitionProps.

supnate commented 1 year ago

Resolved by: https://github.com/eBay/nice-modal-react/commit/5b3dbae4c1f884c2d653bc89416062a514b29132