Refactor logic to prevent breaking changes in useToastState #6270

LFDanLu · 2024-04-26T00:26:43Z

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Test toast stories. Behavior should still be that new toasts are inserted at the bottom of the stack. Toast exit animations should still work

🧢 Your Project:

RSP

rspbot · 2024-04-26T00:42:57Z

Build successful! 🎉

rspbot · 2024-04-26T02:52:56Z

Build successful! 🎉

…into toast_state_take_2

ktabors · 2024-04-26T04:25:52Z

Never mind, I fixed it. ~~Sorry I'm breaking it. I wrote some tests that found one bug that I fixed and a second bug I'll work on tomorrow.~~

The first bug: The close() code assumed all Toasts being closed are visible. When programmatically removing an item that isn't visible the this.visibleToasts.find() returned undefined and we can't set a prop on that.

~~Details about the second unfixed bug:~~

~~There are two Toasts and it removes the first Toast so only the third Toast is left.~~
~~this.visibleItems is empty while this.queue has one item, the third Toast.~~

rspbot · 2024-04-26T04:59:54Z

Build successful! 🎉

rspbot · 2024-04-26T18:24:47Z

Build successful! 🎉

LFDanLu · 2024-04-26T20:25:05Z

packages/@react-stately/toast/src/useToastState.ts

  remove(key: string) {
-    let index = this.queue.findIndex(t => t.key === key);
-    this.visibleToasts = this.visibleToasts.filter(t => t.key !== key);
-    this.updateVisibleToasts(index);
+    this.queue = this.queue.filter(t => t.key !== key);
+    this.updateVisibleToasts('remove');
  }


I'm unsure about whether or not to have this.queue = this.queue.filter(t => t.key !== key); in here or not. The code works without it assuming the user calls close and then remove and would allow us to remove the key input argument. However, if the user calls just remove then we need the aforementioned line to make sure the queue is properly updated and thus visibleToasts gets updated as well.

Technically it does work, the remove() is just doing some cleanup to the dom by removing the object from the dom.

The close button not closing until you move the mouse was related to close() not remove(), right?

Probably good to keep it to be safe.

yeah, the extra update to the queue in the remove call is to ensure the queue as our source of truth is still up to date in case the user only calls remove.

The close button not closing until you move the mouse was related to close() not remove(), right?

yeah it previous happened to use since we weren't always making an update to visibleToast that would cause a state change

I got rid of the extra update to queue in the remove call since this was prematurely removing toasts that get pushed out of visibility by higher priority toasts. In RSP the flow that happens when a toast is pushed out of visibility is as follows:
new toast add animates in -> toast that is no longer visible gets close called by its onAnimationEnd

rspbot · 2024-04-26T20:32:42Z

Build successful! 🎉

devongovett · 2024-04-26T23:58:34Z

packages/@react-stately/toast/src/useToastState.ts

@@ -159,30 +155,27 @@ export class ToastQueue<T> {
    if (index >= 0) {
      this.queue[index].onClose?.();
      this.queue.splice(index, 1);
+      let closedToast = this.visibleToasts.find(t => t.key === key);
+      if (closedToast) {
+        closedToast.animation = 'exiting';


wonder if there will be problems due to mutating the toast, which is passed into props of a component...

devongovett · 2024-04-27T00:04:06Z

packages/@react-stately/toast/src/useToastState.ts

-      this.visibleToasts = toasts;
+    if (action === 'close' && this.hasExitAnimation) {
+      // Cause a rerender to happen for exit animation
+      this.visibleToasts = this.visibleToasts.map(t => t);


One change in logic here is that previously, if the number of visible toasts exceeded the maxVisibleToasts setting, the toasts that got pushed off the end would animate out. Now they won't. Only toasts that are explicitly closed by clicking the X button will animate. Is that expected?

Oh didn't realize that was a case here. Will revisit how we apply the .animation then and see if we can move it back into here somehow

…he visibleToast list due to a higher priority toast being added

packages/@react-stately/toast/src/useToastState.ts

rspbot · 2024-04-29T22:52:41Z

Build successful! 🎉

… from the queue this mimics the exitAnimation call to close that RSP toast does

rspbot · 2024-04-29T23:43:36Z

Build successful! 🎉

…te_take_2

LFDanLu · 2024-04-29T23:51:17Z

You can test the behavior of the toasts with priority in the toast story here on this branch as well: #6290

rspbot · 2024-04-29T23:56:27Z

Build successful! 🎉

ktabors · 2024-04-30T00:07:55Z

packages/@react-stately/toast/src/useToastState.ts

-    } else {
-      this.queue.unshift(toast);
-    }
+    this.queue.splice(low, 0, toast);


Is this a breaking change or was this.queue.unshift(toast); a bug we hadn't found?

This reverts the logic to what is currently released

Right, I just remembered that and was coming back to delete it, but you responded.

rspbot · 2024-04-30T16:43:10Z

Build successful! 🎉

rspbot · 2024-04-30T17:01:31Z

Build successful! 🎉

rspbot · 2024-04-30T17:22:28Z

Build successful! 🎉

devongovett

ok for this release but we are planning to refactor this more later

rspbot · 2024-04-30T23:37:21Z

Build successful! 🎉

rspbot · 2024-04-30T23:37:23Z

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

Refactor logic to prevent breaking changes in useToastState

fbd9172

Merge branch 'main' into toast_state_take_2

b83af04

snowystinger previously approved these changes Apr 26, 2024

View reviewed changes

ktabors added 2 commits April 25, 2024 21:16

added more tests and fixing a bug they caught

6ffd4f5

Merge branch 'toast_state_take_2' of github.com:adobe/react-spectrum …

1e7184a

…into toast_state_take_2

ktabors dismissed snowystinger’s stale review via 1e7184a April 26, 2024 04:18

Fixed the other bug found by new tests

fbde481

LFDanLu changed the title ~~Refactor logic to prevent breaking changes in useToastState~~ (WIP) Refactor logic to prevent breaking changes in useToastState Apr 26, 2024

fix toast visual insertion so it inserts at the bottom

344b124

remove uneeded key and handle case where a user just calls remove

a69022e

LFDanLu changed the title ~~(WIP) Refactor logic to prevent breaking changes in useToastState~~ Refactor logic to prevent breaking changes in useToastState Apr 26, 2024

LFDanLu commented Apr 26, 2024

View reviewed changes

LFDanLu added needs testing release labels Apr 26, 2024

devongovett reviewed Apr 26, 2024

View reviewed changes

devongovett reviewed Apr 27, 2024

View reviewed changes

tentative change to readd exiting to toasts that are shifted out of t…

d812221

…he visibleToast list due to a higher priority toast being added

LFDanLu commented Apr 29, 2024

View reviewed changes

packages/@react-stately/toast/src/useToastState.ts Show resolved Hide resolved

fix so that a toast that is removed but not closed doesnt get removed…

2edead2

… from the queue this mimics the exitAnimation call to close that RSP toast does

Merge branch 'main' of github.com:adobe/react-spectrum into toast_sta…

02cf240

…te_take_2

ktabors previously approved these changes Apr 30, 2024

View reviewed changes

snowystinger previously approved these changes Apr 30, 2024

View reviewed changes

Merge branch 'main' into toast_state_take_2

6ca5d84

Merge branch 'main' into toast_state_take_2

ead9919

devongovett previously approved these changes Apr 30, 2024

View reviewed changes

add test to make sure order of toast doesnt change on close

735fcab

LFDanLu dismissed stale reviews from devongovett, snowystinger, and ktabors via 735fcab April 30, 2024 17:09

snowystinger approved these changes Apr 30, 2024

View reviewed changes

devongovett approved these changes Apr 30, 2024

View reviewed changes

Merge branch 'main' into toast_state_take_2

9026e59

LFDanLu merged commit 1bd538f into main Apr 30, 2024
25 checks passed

LFDanLu deleted the toast_state_take_2 branch April 30, 2024 23:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor logic to prevent breaking changes in useToastState #6270

Refactor logic to prevent breaking changes in useToastState #6270

LFDanLu commented Apr 26, 2024 •

edited

rspbot commented Apr 26, 2024

rspbot commented Apr 26, 2024

ktabors commented Apr 26, 2024 •

edited

rspbot commented Apr 26, 2024

rspbot commented Apr 26, 2024

LFDanLu Apr 26, 2024

ktabors Apr 26, 2024

devongovett Apr 27, 2024

LFDanLu Apr 27, 2024

LFDanLu Apr 29, 2024

rspbot commented Apr 26, 2024

devongovett Apr 26, 2024

devongovett Apr 27, 2024

LFDanLu Apr 27, 2024

rspbot commented Apr 29, 2024

rspbot commented Apr 29, 2024

LFDanLu commented Apr 29, 2024

rspbot commented Apr 29, 2024

ktabors Apr 30, 2024

LFDanLu Apr 30, 2024

ktabors Apr 30, 2024

rspbot commented Apr 30, 2024

rspbot commented Apr 30, 2024

rspbot commented Apr 30, 2024

devongovett left a comment

rspbot commented Apr 30, 2024

rspbot commented Apr 30, 2024

Refactor logic to prevent breaking changes in useToastState #6270

Refactor logic to prevent breaking changes in useToastState #6270

Conversation

LFDanLu commented Apr 26, 2024 • edited

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

rspbot commented Apr 26, 2024

rspbot commented Apr 26, 2024

ktabors commented Apr 26, 2024 • edited

rspbot commented Apr 26, 2024

rspbot commented Apr 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rspbot commented Apr 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rspbot commented Apr 29, 2024

rspbot commented Apr 29, 2024

LFDanLu commented Apr 29, 2024

rspbot commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rspbot commented Apr 30, 2024

rspbot commented Apr 30, 2024

rspbot commented Apr 30, 2024

devongovett left a comment

Choose a reason for hiding this comment

rspbot commented Apr 30, 2024

rspbot commented Apr 30, 2024

LFDanLu commented Apr 26, 2024 •

edited

ktabors commented Apr 26, 2024 •

edited