fix(#2881): temporary notification set 0 to stop auto-dismiss#2882
fix(#2881): temporary notification set 0 to stop auto-dismiss#2882vanessatran-ddi wants to merge 2 commits into
Conversation
|
|
||
| // Verify the notification is displayed | ||
| const notification = result.getByTestId("cancel-notification"); |
There was a problem hiding this comment.
The test failed in headless mode (passed in browser mode) because of a race condition. We query the notification outside the waitFor block, so we query it even before we trigger the temporary notification, that is why it failed.
There was a problem hiding this comment.
I always run in headless mode and it never failed. I am also not clear on what you mean by us querying it before triggering it.
@vanessatran-ddi apparently this test keeps failing, tried couple of times |
@vanessatran-ddi And now it passed. |
|
Hi @chrisolsen This PR needs your code review. |
|
|
||
| // set default duration for certain notification types | ||
| if (!opts.duration && opts.type && TypesRequiringDuration.includes(opts.type)) { | ||
| if (opts.duration === undefined && opts.type && TypesRequiringDuration.includes(opts.type)) { |
There was a problem hiding this comment.
This works as well, but what is the reason for the change?
There was a problem hiding this comment.
When we pass 0 as a number, the !opts.duration is true, and it doesn't work.
|
|
||
| // Verify the notification is displayed | ||
| const notification = result.getByTestId("cancel-notification"); |
There was a problem hiding this comment.
I always run in headless mode and it never failed. I am also not clear on what you mean by us querying it before triggering it.
Thank you @syedszeeshan Then it is kind of flaky.. I don't know, maybe we talk about it in the dev meeting? |
|
We don't allow |
3 participants
Before (the change)
After (the change)
It will stop auto-dismiss the notification.
Make sure that you've checked the boxes below before you submit the PR
Steps needed to test