fix(OverlayToaster): Fix toasts being cut off if show() called too quickly #7049
Conversation
Generate changelog in
|
Fix commentBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
| toasts: ToastOptions[]; | ||
| } | ||
|
|
||
| const QUEUE_TIMEOUT_MS = 50; |
There was a problem hiding this comment.
What determines that the timeout should be 50ms? Is this something that we should potentially make configurable?
There was a problem hiding this comment.
This was based on experimentation on my part, it was a value that seemed to never exhibit issues with this problem (+ some buffer to be safe) and not have too much apparent effect on the UX of the component. I don't see a particular reason right now to make this configurable.
| toaster.show({ message: "one" }); | ||
| toaster.show({ message: "two" }); | ||
| toaster.show({ message: "six" }); | ||
| await delay(150); |
There was a problem hiding this comment.
Is there any way we can tie these hardcoded values to the value of QUEUE_TIMEOUT_MS? That might make them a bit less ambiguous in the context of these tests. Perhaps we could leave a comment or even export reference the constant itself in code:
await delay(3 * QUEUE_TIMEOUT_MS);Also, should we be using timer mocks in our tests?
There was a problem hiding this comment.
Yeah good call on the delay, will export that. I attempted to user the timer mocks and I wasn't able to get them to work, will give them another shot.
There was a problem hiding this comment.
Figured out what I messed up and switched to fake timers 👍
| } | ||
|
|
||
| private handleQueueTimeout = () => { | ||
| const nextToast = this.queue.toasts.shift(); | ||
| this.queue.isRunning = false; |
There was a problem hiding this comment.
Do we have to set isRunning to false during each execution of the timeout? Would it make sense to check that the queue is empty first? Something like:
if (this.queue.toasts.length === 0) {
this.queue.isRunning = false;
}There was a problem hiding this comment.
Yeah good call. I ended up implementing this slightly differently but with the same intent.
CRBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Fixes #7034
Checklist
Changes proposed in this pull request:
This issue appears to be a react css transition group issue, but we can mitigate it on our side by effectively debouncing the
show()call. Now when a user callsshow()if there's been a toasted shown in the last 50ms, the new toast will be added to a queue and only shown after that time has elapsed. Any othershow()calls that happen within that window will also be added to the queue and only one new toast will be shown after each 50ms increment.Screenshots
Without this change we get this ugly behavior:
With the change it animates in properly:
