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 7 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
4 changes: 2 additions & 2 deletions packages/@react-spectrum/toast/src/toastContainer.css
Expand Up @@ -29,14 +29,14 @@

&[data-position=top] {
top: 0;
flex-direction: column;
flex-direction: column-reverse;
--slide-from: translateY(-100%);
--slide-to: translateY(0);
}

&[data-position=bottom] {
bottom: 0;
flex-direction: column-reverse;
flex-direction: column;
--slide-from: translateY(100%);
--slide-to: translateY(0);
}
Expand Down
33 changes: 13 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('add');
return toastKey;
}

Expand All @@ -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';
Copy link
Member

Choose a reason for hiding this comment

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

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

}
}

this.updateVisibleToasts(index);
this.updateVisibleToasts('close');
}

/** 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.queue = this.queue.filter(t => t.key !== key);
this.updateVisibleToasts('remove');
}
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Probably good to keep it to be safe.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


private updateVisibleToasts(oldIndex = -1) {
private updateVisibleToasts(action: 'add' | 'close' | 'remove') {
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 === '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
135 changes: 131 additions & 4 deletions packages/@react-stately/toast/test/useToastState.test.js
Expand Up @@ -19,6 +19,14 @@ describe('useToastState', () => {
props: {timeout: 0}
}];

beforeEach(() => {
jest.useFakeTimers();
});

afterEach(() => {
act(() => jest.runAllTimers());
});

it('should add a new toast via add', () => {
let {result} = renderHook(() => useToastState());
expect(result.current.visibleToasts).toStrictEqual([]);
Expand Down Expand Up @@ -58,8 +66,127 @@ 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 be able to display three toasts and remove the middle toast via timeout then the visible toast', () => {
let {result} = renderHook(() => useToastState({maxVisibleToasts: 3}));

// Add the first toast
act(() => {
result.current.add('First Toast', {timeout: 0});
});
expect(result.current.visibleToasts).toHaveLength(1);
expect(result.current.visibleToasts[0].content).toBe('First Toast');

// Add the second toast
act(() => {
result.current.add('Second Toast', {timeout: 1000});
});
expect(result.current.visibleToasts).toHaveLength(2);
expect(result.current.visibleToasts[0].content).toBe('First Toast');

result.current.resumeAll();

// Add the third toast
act(() => {
result.current.add('Third Toast', {timeout: 0});
});
expect(result.current.visibleToasts).toHaveLength(3);
expect(result.current.visibleToasts[0].content).toBe('First Toast');
expect(result.current.visibleToasts[1].content).toBe('Second Toast');
expect(result.current.visibleToasts[2].content).toBe('Third Toast');

act(() => jest.advanceTimersByTime(500));
expect(result.current.visibleToasts).toHaveLength(3);

act(() => jest.advanceTimersByTime(1000));
expect(result.current.visibleToasts).toHaveLength(2);
expect(result.current.visibleToasts[0].content).toBe('First Toast');
expect(result.current.visibleToasts[1].content).toBe('Third Toast');

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

it('should be able to display one toast without exitAnimation, add multiple toasts, and remove the middle not visible one programmatically', () => {
let {result} = renderHook(() => useToastState());

// Add the first toast
act(() => {
result.current.add('First Toast', {timeout: 0});
});
expect(result.current.visibleToasts).toHaveLength(1);
expect(result.current.visibleToasts[0].content).toBe('First Toast');

let secondToastKey = null;
// Add the second toast
act(() => {
secondToastKey = result.current.add('Second Toast', {timeout: 0});
});
expect(result.current.visibleToasts).toHaveLength(1);
expect(result.current.visibleToasts[0].content).toBe('First Toast');

// Add the third toast
act(() => {
result.current.add('Third Toast', {timeout: 0});
});
expect(result.current.visibleToasts).toHaveLength(1);
expect(result.current.visibleToasts[0].content).toBe('First Toast');

// Remove a toast that isn't visible
act(() => {result.current.close(secondToastKey);});
expect(result.current.visibleToasts).toHaveLength(1);
expect(result.current.visibleToasts[0].content).toBe('First Toast');

// Remove the visible toast to confirm the middle toast was removed
act(() => {result.current.close(result.current.visibleToasts[0].key);});
expect(result.current.visibleToasts.length).toBe(1);
expect(result.current.visibleToasts[0].content).toBe('Third Toast');
});

it('should be able to display one toast with exitAnimation, add multiple toasts, and remove the middle not visible one programmatically', () => {
let {result} = renderHook(() => useToastState({hasExitAnimation: true}));

// Add the first toast
act(() => {
result.current.add('First Toast', {timeout: 0});
});
expect(result.current.visibleToasts).toHaveLength(1);
expect(result.current.visibleToasts[0].content).toBe('First Toast');

let secondToastKey = null;
// Add the second toast
act(() => {
secondToastKey = result.current.add('Second Toast', {timeout: 0});
});
expect(result.current.visibleToasts).toHaveLength(1);
expect(result.current.visibleToasts[0].content).toBe('First Toast');

// Add the third toast
act(() => {
result.current.add('Third Toast', {timeout: 0});
});
expect(result.current.visibleToasts).toHaveLength(1);
expect(result.current.visibleToasts[0].content).toBe('First Toast');

// Remove a toast that isn't visible
act(() => {result.current.close(secondToastKey);});
expect(result.current.visibleToasts).toHaveLength(1);
expect(result.current.visibleToasts[0].content).toBe('First Toast');

// Remove the visible toast to confirm the middle toast was removed
act(() => {result.current.close(result.current.visibleToasts[0].key);});
expect(result.current.visibleToasts.length).toBe(1);
expect(result.current.visibleToasts[0].content).toBe('First Toast');
expect(result.current.visibleToasts[0].animation).toBe('exiting');
act(() => {result.current.remove(result.current.visibleToasts[0].key);});

// there should only be one Toast left, the third one
expect(result.current.visibleToasts.length).toBe(1);
expect(result.current.visibleToasts[0].content).toBe('Third Toast');
});

it('should close a toast', () => {
Expand Down Expand Up @@ -91,11 +218,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