fix(manager-react-components): useNotifications prevent early clears #14234
Conversation
tristanwagner
requested review from
frenautvh,
seven-amid,
antonymarion,
anooparveti and
JacquesLarique
| await new Promise((resolve) => setTimeout(resolve, 2000)); | ||
|
|
||
| render(<ClearNotifications />); | ||
| container = render(<Notifications />).container; | ||
|
|
||
| await waitFor(() => { | ||
| expect(container.children.length).toBe(0); | ||
| }); |
There was a problem hiding this comment.
I think it's better mock Date.
- It's a unit test we don't test Data.now() method
- This allow to do the test faster
There was a problem hiding this comment.
enhanced the tests according to your suggestions
| clearNotifications: () => | ||
| set((state) => ({ | ||
| notifications: state.notifications.filter( | ||
| (notification) => Date.now() - notification.creationTimestamp < 1000, |
There was a problem hiding this comment.
| (notification) => Date.now() - notification.creationTimestamp < 1000, | |
| (notification) => Date.now() - notification.creationTimestamp < DEFAULT_MINIMUN_LIFETIME, |
Optionnal suggestion : Possible to add constant for default minimum lifetime and maybe add parameters if we want more or less on the future ?
There was a problem hiding this comment.
To me the whole api should be reworked, for example we should be able to set "clearAfterRead" per notification instead of for all of them, but I think it should be another subject, right now I'm just trying to be up to date with MRC v1 on this issue
There was a problem hiding this comment.
FYI we opened this PR to sync the changes we introduced in MRC 1.x : #14170
There was a problem hiding this comment.
Oh ok wasn't aware of that I'll just close this one then
tristanwagner
force-pushed
the
fix/notification-early-clear
branch
from
659448e
to
ac4ffbf
Compare
Signed-off-by: Florian Renaut <[email protected]> Signed-off-by: Tristan WAGNER <[email protected]>
tristanwagner
force-pushed
the
fix/notification-early-clear
branch
from
ac4ffbf
to
6bbe102
Compare
|
|
should be handled by #14170 |
this is a cherry pick of this commit on mrc v1 maintenance branch
developDescription
Related