Skip to content

[notifications] Fix: #4959 transition on close #4960

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

dchae
Copy link

@dchae dchae commented May 18, 2025

  • I've read and followed the contributing guide on how to create great pull requests.
  • I've updated the relevant documentation for any new or updated feature.
  • I've linked relevant GitHub issue with "Closes #".
  • I've added a visual demonstration in the form of a screenshot or video.

Fixes: #4959

dchae added 2 commits May 19, 2025 04:35
- `close` now sets the notification's `open` prop to `false` before
removing it from the queue.
@mui-bot
Copy link

mui-bot commented May 18, 2025

Netlify deploy preview

https://deploy-preview-4960--mui-toolpad-docs.netlify.app/

Generated by 🚫 dangerJS against 685eebb

@bharatkashyap
Copy link
Member

bharatkashyap commented May 19, 2025

Thanks for contributing! Works, tested with https://codesandbox.io/p/devbox/nervous-fire-tyyrhm?workspaceId=ws_8roDQkQB8Gsa4Et8HYWgP6

@Janpot impacts the useNotifications code, to have a look

@bharatkashyap bharatkashyap added bug 🐛 Something doesn't work design: ux Design labels May 19, 2025
@bharatkashyap bharatkashyap requested a review from Janpot May 19, 2025 07:07
@dchae
Copy link
Author

dchae commented May 23, 2025

@Janpot I'm happy to refactor this if you have feedback btw!

@Janpot
Copy link
Member

Janpot commented May 23, 2025

Instead of a timeout, I wonder if this can use onExited of the transition instead?

@dchae
Copy link
Author

dchae commented May 23, 2025

That sounds much cleaner, thank you - I'll give it a go

dchae added 3 commits May 24, 2025 06:34
…ications queue

- Also refactored `remove` to be consistent with the other context
methods. Possible this version is easier for React to optimise?
@@ -90,10 +93,25 @@ function Notification({ notificationKey, open, message, options, badge }: Notifi

const props = React.useContext(RootPropsContext);
const SnackbarComponent = props?.slots?.snackbar ?? Snackbar;

// Passing `onExited` through `externalSlotProps` here.
Copy link
Author

Choose a reason for hiding this comment

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

@Janpot This is not ideal, but I couldn't think of a better way that didn't involve rewriting mergeSlotProps. Let me know if I'm missing something - I'm fairly new to React and mui.

@mnemotiv
Copy link

is this going to be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work design: ux Design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useNotifications notifications do not transition on close or autoHide
5 participants