Skip to content

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

Open
dchae wants to merge 6 commits intomui:masterfrom
dchae:useNotifications-transition-fix
Open

[notifications] Fix: #4959 transition on close#4960
dchae wants to merge 6 commits intomui:masterfrom
dchae:useNotifications-transition-fix

Conversation

@dchae
Copy link
Copy Markdown

@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
Copy Markdown

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
Copy Markdown
Collaborator

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 type: bug It doesn't behave as expected. design: ux Related to End-users' experience. labels May 19, 2025
@bharatkashyap bharatkashyap requested a review from Janpot May 19, 2025 07:07
@dchae
Copy link
Copy Markdown
Author

dchae commented May 23, 2025

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

@Janpot
Copy link
Copy Markdown
Member

Janpot commented May 23, 2025

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

@dchae
Copy link
Copy Markdown
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?
const props = React.useContext(RootPropsContext);
const SnackbarComponent = props?.slots?.snackbar ?? Snackbar;

// Passing `onExited` through `externalSlotProps` here.
Copy link
Copy Markdown
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
Copy Markdown

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

design: ux Related to End-users' experience. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

useNotifications notifications do not transition on close or autoHide

5 participants