-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Fix <Notification> causes undoable notifications to be commited twice in MUI v6
#11345
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,91 @@ | ||
| import * as React from 'react'; | ||
| import { fireEvent, render, screen, waitFor } from '@testing-library/react'; | ||
|
|
||
| let mockMuiMajor = 5; | ||
|
|
||
| jest.mock('@mui/material', () => { | ||
| const actual = jest.requireActual('@mui/material'); | ||
| const ReactModule = jest.requireActual('react') as typeof React; | ||
| const Mui6Snackbar = ReactModule.forwardRef<any, any>((props, ref) => { | ||
| const wasOpen = ReactModule.useRef(props.open); | ||
|
|
||
| ReactModule.useEffect(() => { | ||
| if (wasOpen.current && !props.open) { | ||
| props.slotProps?.transition?.onExited?.(null); | ||
| props.TransitionProps?.onExited?.(null); | ||
| } | ||
| wasOpen.current = props.open; | ||
| }, [props.open, props.slotProps, props.TransitionProps]); | ||
|
|
||
| ReactModule.useEffect(() => { | ||
| if (!props.open) return; | ||
|
|
||
| document.addEventListener('click', props.onClose); | ||
| return () => document.removeEventListener('click', props.onClose); | ||
| }, [props.open, props.onClose]); | ||
|
|
||
| if (!props.open) return null; | ||
|
|
||
| return ReactModule.createElement( | ||
| 'div', | ||
| { ref }, | ||
| props.children ?? props.message, | ||
| props.action, | ||
| ReactModule.createElement( | ||
| 'button', | ||
| { onClick: props.onClose }, | ||
| 'Close notification' | ||
| ) | ||
| ); | ||
| }); | ||
| const Snackbar = ReactModule.forwardRef<any, any>((props, ref) => | ||
| mockMuiMajor >= 6 | ||
| ? ReactModule.createElement(Mui6Snackbar, { ...props, ref }) | ||
| : ReactModule.createElement(actual.Snackbar, { ...props, ref }) | ||
| ); | ||
|
|
||
| return { | ||
| ...actual, | ||
| major: { valueOf: () => mockMuiMajor }, | ||
| Snackbar, | ||
| }; | ||
| }); | ||
|
|
||
| import { | ||
| ConsecutiveNotifications, | ||
| ConsecutiveUndoable, | ||
| CustomNotificationWithAction, | ||
| } from './Notification.stories'; | ||
|
|
||
| describe('<Notification />', () => { | ||
| afterEach(() => { | ||
| mockMuiMajor = 5; | ||
| }); | ||
|
|
||
| it.each([6, 7, 9])( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that it makes much sense to test all versions. I'd prefer that you create a test that only runs if the MUI major it >=6, with no mock. And then explain in the PR description that, in order to test you change, te tests must pass with devdeps using mui V5 and mui V6.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in 3eaa674. The regression now runs only when the installed MUI major is 6 or newer, with no MUI mock. I verified the file against MUI 5.16.14 (3 passed, 1 expected skip) and MUI 6.4.10 (4 passed), and updated the PR description with the two-version test requirement. |
||
| 'should confirm an undoable mutation once when the notification exits with MUI %s', | ||
| async muiMajor => { | ||
| mockMuiMajor = muiMajor; | ||
| const deleteOne = jest | ||
| .fn() | ||
| .mockImplementation((_resource, { id }) => | ||
| Promise.resolve({ data: { id } }) | ||
| ); | ||
| const dataProvider = { delete: deleteOne } as any; | ||
| render(<ConsecutiveUndoable dataProvider={dataProvider} />); | ||
|
|
||
| (await screen.findByText('Delete post 1')).click(); | ||
| await screen.findByText('Post 1 deleted'); | ||
|
|
||
| fireEvent.click( | ||
| screen.getByRole('button', { name: 'Close notification' }) | ||
| ); | ||
|
|
||
| await waitFor(() => expect(deleteOne).toHaveBeenCalled()); | ||
| expect(deleteOne).toHaveBeenCalledTimes(1); | ||
| } | ||
| ); | ||
|
|
||
| it('should confirm the first undoable notification when a second one starts', async () => { | ||
| const deleteOne = jest | ||
| .fn() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't understand why you need to reimplement the Snackbar to test mui v6. Can you explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right; reimplementing Snackbar was unnecessary. Removed the module mock in 3eaa674. The regression now exercises the real MUI Snackbar and triggers its click-away exit path.