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

onExited callback key mismatch caused by Material-UI #276

Closed
Gareon opened this issue May 20, 2020 · 8 comments
Closed

onExited callback key mismatch caused by Material-UI #276

Gareon opened this issue May 20, 2020 · 8 comments

Comments

@Gareon
Copy link

Gareon commented May 20, 2020

Expected Behavior

When enqueuing a snackbar providing a key as well as a callback for the onExited event, I expected the key given as the second callback argument when it gets fired.

Current Behavior

The onExited event gets fired, but the key argument is always undefined.

Steps to Reproduce

  1. Enqueue a keyed snackbar with a onExited handler and inspect the key argument as soon as it gets fired:
    enqueueSnackbar("Some message", {
      key: "test",
      variant: "default",
      onExited: (_, myKey) => {
        console.info("exited with key=" + JSON.stringify(myKey));
      }
    });
  2. Inspect console output.
  3. Behold the message: exited with key=undefined

You can reproduce / verify the problem by checking out the redux integration example on codesandbox.io.

Context

According to the documentation, I'm trying to integrate notistack with a redux state.

Your Environment

Tech Version
Notistack v0.9.16
React v16.13.1
Browser Google Chrome (v81.0.4044.129)
Material-UI v4.9.14
@r-tae
Copy link

r-tae commented May 22, 2020

Hey @Gareon, I found your issue from noticing the same behaviour and it turns out that onExited actually has three arguments.

There are still other things wrong with it, but you can fix any issues you're having with the key by doing onExited: (_, _, myKey) => { }

@iamhosseindhv
Copy link
Owner

This is bug caused by latest version of material-ui. meaning if you downgrade MUI core it'll start working:
mui/material-ui#21091

Alternatively and temporarily, as @actual-size said, you can access the key as the third argument.

@iamhosseindhv iamhosseindhv changed the title onExited callback does always provide undefined as key argument onExited callback key mismatch caused by Material-UI May 22, 2020
@iamhosseindhv
Copy link
Owner

I'm going to open a PR in mui-core to fix this. Until then I'm keeping this open.

@oliviertassinari
Copy link
Contributor

oliviertassinari commented May 23, 2020

If I understand the previous API was:

enqueueSnackbar("Some message", {
  key: "test",
  variant: "default",
  onEntered: (node, appearing, key) => {
    console.info("exited with key=" + JSON.stringify(myKey));
  },
  onExited: (node, key) => {
    console.info("exited with key=" + JSON.stringify(myKey));
  }
});

Would it make sense to turn the API to an object?

enqueueSnackbar("Some message", {
  key: "test",
  variant: "default",
  onEntered: ({ node, appearing, key, variant }) => {
    console.info("exited with key=" + JSON.stringify(myKey));
  },
  onExited: ({ node, key, variant }) => {
    console.info("exited with key=" + JSON.stringify(myKey));
  }
});

@iamhosseindhv
Copy link
Owner

@oliviertassinari It'd make sense to pass node, isAppearing and key in one object. But it's a breaking change.

Also did you intend to include variant and possibly other options in the callback or was it accidentally added?

@oliviertassinari
Copy link
Contributor

@iamhosseindhv Definitely, I was wondering about the merit of the change for a future major. Regarding variant, I wanted to illustrate the future use cases it could allow to support, not sure this prop is valuable specifically.

@iamhosseindhv
Copy link
Owner

iamhosseindhv commented May 23, 2020

I agree @oliviertassinari. We have a similar situation in #259 were people need more options to be passed in the callback. Possibly a good feature to have an eye on.

@iamhosseindhv
Copy link
Owner

Closing this as my PR mui/material-ui#21158 is now merged.

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

No branches or pull requests

4 participants