Skip to content

ix-notifications-dom-pollution-8470 #8490

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.

Sign up for GitHub

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

Closed
saurabh24thakur wants to merge 1 commit into from
Closed

ix-notifications-dom-pollution-8470 #8490

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 21 additions & 31 deletions packages/@mantine/notifications/src/Notifications.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,37 +217,27 @@

return (
<OptionalPortal withinPortal={withinPortal} {...portalProps}>
<Box {...getStyles('root')} data-position="top-center" ref={ref} {...others}>
<TransitionGroup>{groupedComponents['top-center']}</TransitionGroup>
</Box>

<Box {...getStyles('root')} data-position="top-left" {...others}>
<TransitionGroup>{groupedComponents['top-left']}</TransitionGroup>
</Box>

<Box
{...getStyles('root', { className: RemoveScroll.classNames.fullWidth })}
data-position="top-right"
{...others}
>
<TransitionGroup>{groupedComponents['top-right']}</TransitionGroup>
</Box>

<Box
{...getStyles('root', { className: RemoveScroll.classNames.fullWidth })}
data-position="bottom-right"
{...others}
>
<TransitionGroup>{groupedComponents['bottom-right']}</TransitionGroup>
</Box>

<Box {...getStyles('root')} data-position="bottom-left" {...others}>
<TransitionGroup>{groupedComponents['bottom-left']}</TransitionGroup>
</Box>

<Box {...getStyles('root')} data-position="bottom-center" {...others}>
<TransitionGroup>{groupedComponents['bottom-center']}</TransitionGroup>
</Box>
{positions.map((pos, index) => {
const hasNotifications = grouped[pos].length > 0;
if (!hasNotifications) return null;

Check failure on line 222 in packages/@mantine/notifications/src/Notifications.tsx

View workflow job for this annotation

GitHub Actions / test_pull_request 

Expected { after 'if' condition
Copy link
Member

@rtivital rtivital Nov 25, 2025

Choose a reason for hiding this comment

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

This does not work correctly with react-transition-group – when the last notification is removed the animation does not work.

Copy link
Author

@saurabh24thakur saurabh24thakur Nov 26, 2025

Choose a reason for hiding this comment

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

Thank you for the feedback! You're absolutely right about the animation issue.
After investigating, I realize that properly fixing this while maintaining TransitionGroup's exit animations is more complex than initially expected. The container needs to stay mounted during the exit transition, which conflicts with the goal of removing empty containers from the DOM.
Given that:

  • The 6 empty divs have minimal performance impact
  • Fixing this properly requires significant complexity with react-transition-group
  • The main issue was test diff noise, which can be addressed with better test selectors
    I think it might be best to close this issue as "won't fix" for now. What do you think?
    Alternatively, if you have a preferred approach for handling this with TransitionGroup, I'm happy to implement it!


const isFullWidth = pos === 'top-right' || pos === 'bottom-right';
const isFirstContainer = index === 0;

return (
<Box
key={pos}
{...getStyles('root', {
className: isFullWidth ? RemoveScroll.classNames.fullWidth : undefined,
})}
data-position={pos}
ref={isFirstContainer ? ref : undefined}
{...others}
>
<TransitionGroup>{groupedComponents[pos]}</TransitionGroup>
</Box>
);
})}
</OptionalPortal>
);
});
Expand Down
Loading