Skip to content
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

Refactor logic to prevent breaking changes in useToastState #6270

Merged
merged 14 commits into from
Apr 30, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/@react-spectrum/toast/src/toastContainer.css
Expand Up @@ -21,6 +21,7 @@
pointer-events: none;
outline: none;
margin-block-end: 8px;
flex-direction: column-reverse;

.spectrum-Toast {
margin: 8px;
Expand Down
32 changes: 12 additions & 20 deletions packages/@react-stately/toast/src/useToastState.ts
Expand Up @@ -134,19 +134,15 @@ export class ToastQueue<T> {
}
}

if (toast.priority) {
this.queue.splice(low, 0, toast);
} else {
this.queue.unshift(toast);
}
this.queue.splice(low, 0, toast);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reverts the logic to what is currently released

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


toast.animation = low < this.maxVisibleToasts ? 'entering' : 'queued';
let i = this.maxVisibleToasts;
while (i < this.queue.length) {
this.queue[i++].animation = 'queued';
}

this.updateVisibleToasts();
this.updateVisibleToasts({action: 'add'});
return toastKey;
}

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

this.updateVisibleToasts(index);
this.updateVisibleToasts({action: 'close', key});
}

/** Removes a toast from the visible toasts after an exiting animation. */
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.updateVisibleToasts({action: 'remove', key});
}

private updateVisibleToasts(oldIndex = -1) {
private updateVisibleToasts(options: {action: 'add' | 'close' | 'remove', key?: string}) {
let {action, key} = options;
let toasts = this.queue.slice(0, this.maxVisibleToasts);
if (this.hasExitAnimation) {
let prevToasts: QueuedToast<T>[] = this.visibleToasts
.filter(t => !toasts.some(t2 => t.key === t2.key))
.map(t => ({...t, animation: 'exiting'}));

if (oldIndex !== -1) {
toasts.splice(oldIndex, 0, prevToasts?.[0]);
}

this.visibleToasts = toasts;
if (action === 'remove') {
this.visibleToasts = this.visibleToasts.filter(t => t.key !== key);
} else if (action === 'close' && this.hasExitAnimation) {
// Cause a rerender to happen for exit animation
this.visibleToasts = this.visibleToasts.map(t => t);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
this.visibleToasts = toasts;
}
Expand Down
8 changes: 4 additions & 4 deletions packages/@react-stately/toast/test/useToastState.test.js
Expand Up @@ -58,8 +58,8 @@ describe('useToastState', () => {

act(() => {result.current.add(secondToast.content, secondToast.props);});
expect(result.current.visibleToasts.length).toBe(2);
expect(result.current.visibleToasts[0].content).toBe(secondToast.content);
expect(result.current.visibleToasts[1].content).toBe(newValue[0].content);
expect(result.current.visibleToasts[0].content).toBe(newValue[0].content);
expect(result.current.visibleToasts[1].content).toBe(secondToast.content);
});

it('should close a toast', () => {
Expand Down Expand Up @@ -91,11 +91,11 @@ describe('useToastState', () => {

act(() => {result.current.add('Second Toast');});
expect(result.current.visibleToasts.length).toBe(1);
expect(result.current.visibleToasts[0].content).toBe('Second Toast');
expect(result.current.visibleToasts[0].content).toBe(newValue[0].content);

act(() => {result.current.close(result.current.visibleToasts[0].key);});
expect(result.current.visibleToasts.length).toBe(1);
expect(result.current.visibleToasts[0].content).toBe(newValue[0].content);
expect(result.current.visibleToasts[0].content).toBe('Second Toast');
expect(result.current.visibleToasts[0].animation).toBe('queued');
});

Expand Down