10000 fix: Improve current overlay determination logic by jungpaeng · Pull Request #113 · toss/overlay-kit · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: Improve current overlay determination logic #113

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

Merged
merged 6 commits into from
Feb 11, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .changeset/clever-buses-juggle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
"overlay-kit": patch
---

Improve overlay unmount logic and add test cases

- Enhanced current overlay state management during unmount
- When unmounting a middle overlay with multiple overlays open, the last overlay becomes current
- When unmounting the last overlay, the previous overlay becomes current
- Added test cases
- Test for unmounting multiple overlays in different orders
- Test for tracking current overlay state using useCurrentOverlay hook
1 change: 0 additions & 1 deletion packages/src/context/index.ts

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export function ContentOverlayController({
/**
* @description Executes when closing and reopening an overlay without unmounting.
*/
if (prevCurrent.current !== current) {
if (prevCurrent.current !== current && isOpen === false) {
prevCurrent.current = current;

if (current === overlayId) {
Expand Down
35 changes: 20 additions & 15 deletions packages/src/context/reducer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,23 +85,28 @@ export function overlayReducer(state: OverlayData, action: OverlayReducerAction)
const copiedOverlayData = { ...state.overlayData };
delete copiedOverlayData[action.overlayId];

const current = state.current
? remainingOverlays.includes(state.current)
? /**
* @description If `unmount` was executed after `close`
*/
state.current
: /**
* @description If you only run `unmount`, there is no `current` in `remainingOverlays`
*/
remainingOverlays.at(-1) ?? null
: /**
* @description The case where `current` is `null`
*/
null;
const openedOverlayOrderList = state.overlayOrderList.filter(
(orderedOverlayId) => state.overlayData[orderedOverlayId].isOpen === true
);
const targetIndexInOpenedList = openedOverlayOrderList.findIndex((item) => item === action.overlayId);

/**
* @description If unmounting the last overlay, specify the overlay before it.
* @description If unmounting intermediate overlays, specifies the last overlay.
*
* @example open - [1, 2, 3, 4]
* unmount 2 => current: 4
* unmount 4 => current: 3
* unmount 3 => current: 1
* unmount 1 => current: null
*/
const currentOverlayId =
targetIndexInOpenedList === openedOverlayOrderList.length - 1
? openedOverlayOrderList[targetIndexInOpenedList - 1] ?? null
: openedOverlayOrderList.at(-1) ?? null;

return {
current,
current: currentOverlayId,
overlayOrderList: remainingOverlays,
overlayData: copiedOverlayData,
};
Expand Down
1 change: 0 additions & 1 deletion packages/src/context/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ export function createRegisterOverlaysStore() {
};

function dispatchOverlay(action: OverlayReducerAction) {
console.log('test:: dispatchOverlay Before', action, overlays);
overlays = overlayReducer(overlays, action);
console.log('test:: dispatchOverlay After', action, overlays);
emitChangeListener();
Expand Down
178 changes: 144 additions & 34 deletions packages/src/event.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { render, screen, waitFor } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import React, { useEffect, type PropsWithChildren } from 'react';
import { describe, expect, it, vi } from 'vitest';
import { OverlayProvider, overlay } from './utils/create-overlay-context';
import { OverlayProvider, overlay, useCurrentOverlay } from './utils/create-overlay-context';

function wrapper({ children }: PropsWithChildren) {
return <OverlayProvider>{children}</OverlayProvider>;
Expand All @@ -24,27 +24,20 @@ describe('overlay object', () => {

function Component() {
useEffect(() => {
overlay.open(({ overlayId }) => {
return (
<button
=> {
overlay.unmount(overlayId);
}}
>
{overlayDialogContent}
</button>
);
< 8000 /td> overlay.open(({ isOpen, overlayId }) => {
return isOpen && <button => overlay.unmount(overlayId)}>{overlayDialogContent}</button>;
});
}, []);

return <div>Empty</div>;
}

const { user } = renderWithUser(<Component />);

await user.click(await screen.findByRole('button', { name: overlayDialogContent }));

expect(screen.queryByRole('button', { name: overlayDialogContent })).not.toBeInTheDocument();
await waitFor(() => {
expect(screen.queryByRole('button', { name: overlayDialogContent })).not.toBeInTheDocument();
});
});

it('should be able to open multiple overlays via overlay.open', async () => {
Expand All @@ -55,28 +48,31 @@ describe('overlay object', () => {

function Component() {
useEffect(() => {
overlay.open(() => {
return <p>{testContent1}</p>;
overlay.open(({ isOpen }) => {
return isOpen && <p>{testContent1}</p>;
});
overlay.open(() => {
return <p>{testContent2}</p>;
overlay.open(({ isOpen }) => {
return isOpen && <p>{testContent2}</p>;
});
overlay.open(() => {
return <p>{testContent3}</p>;
overlay.open(({ isOpen }) => {
return isOpen && <p>{testContent3}</p>;
});
overlay.open(() => {
return <p>{testContent4}</p>;
overlay.open(({ isOpen }) => {
return isOpen && <p>{testContent4}</p>;
});
}, []);

return <div>Empty</div>;
}

render(<Component />, { wrapper });
expect(screen.queryByText(testContent1)).toBeInTheDocument();
expect(screen.queryByText(testContent2)).toBeInTheDocument();
expect(screen.queryByText(testContent3)).toBeInTheDocument();
expect(screen.queryByText(testContent4)).toBeInTheDocument();

await waitFor(() => {
expect(screen.queryByText(testContent1)).toBeInTheDocument();
expect(screen.queryByText(testContent2)).toBeInTheDocument();
expect(screen.queryByText(testContent3)).toBeInTheDocument();
expect(screen.queryByText(testContent4)).toBeInTheDocument();
});
});

it('The value passed as an argument to close is passed to resolve. overlay.openAsync', async () => {
Expand All @@ -88,9 +84,9 @@ describe('overlay object', () => {
return (
<button
() => {
const result = await overlay.openAsync<boolean>(({ close }) => (
<button => close(true)}>{overlayDialogContent}</button>
));
const result = await overlay.openAsync<boolean>(
({ isOpen, close }) => isOpen && <button => close(true)}>{overlayDialogContent}</button>
);

if (result) {
mockFn(result);
Expand All @@ -103,9 +99,7 @@ describe('overlay object', () => {
}

const { user } = renderWithUser(<Component />);

await user.click(await screen.findByRole('button', { name: overlayTriggerContent }));

await user.click(await screen.findByRole('button', { name: overlayDialogContent }));

await waitFor(() => {
Expand All @@ -121,8 +115,8 @@ describe('overlay object', () => {
return (
<button
() => {
overlay.openAsync<boolean>(({ isOpen, close }) =>
isOpen ? <button => close(true)}>{overlayDialogContent}</button> : null
overlay.openAsync<boolean>(
({ isOpen, close }) => isOpen && <button => close(true)}>{overlayDialogContent}</button>
);
}}
>
Expand All @@ -132,13 +126,129 @@ describe('overlay object', () => {
}

const { user } = renderWithUser(<Component />, { wrapper });

await user.click(await screen.findByRole('button', { name: overlayTriggerContent }));

await user.click(await screen.findByRole('button', { name: overlayDialogContent }));

await waitFor(() => {
expect(screen.queryByRole('button', { name: overlayDialogContent })).not.toBeInTheDocument();
});
});

it('should handle current overlay correctly when unmounting overlays in different orders', async () => {
const contents = {
first: 'overlay-content-1',
second: 'overlay-content-2',
third: 'overlay-content-3',
fourth: 'overlay-content-4',
};

let overlayIds: string[] = [];

function Component() {
useEffect(() => {
// Open 4 overlays sequentially
overlayIds = [
overlay.open(({ isOpen }) => isOpen && <div data-testid="overlay-1">{contents.first}</div>),
overlay.open(({ isOpen }) => isOpen && <div data-testid="overlay-2">{contents.second}</div>),
overlay.open(({ isOpen }) => isOpen && <div data-testid="overlay-3">{contents.third}</div>),
overlay.open(({ isOpen }) => isOpen && <div data-testid="overlay-4">{contents.fourth}</div>),
];
}, []);

return <div>Base Component</div>;
}

render(<Component />, { wrapper });

// Wait for all overlays to be mounted
await waitFor(() => {
expect(screen.getByTestId('overlay-1')).toBeInTheDocument();
expect(screen.getByTestId('overlay-2')).toBeInTheDocument();
expect(screen.getByTestId('overlay-3')).toBeInTheDocument();
expect(screen.getByTestId('overlay-4')).toBeInTheDocument();
});

// Remove middle overlay (2)
overlay.unmount(overlayIds[1]);
await waitFor(() => {
expect(screen.queryByTestId('overlay-2')).not.toBeInTheDocument();
expect(screen.getByTestId('overlay-1')).toBeVisible();
expect(screen.getByTestId('overlay-3')).toBeVisible();
expect(screen.getByTestId('overlay-4')).toBeVisible();
});

// Remove last overlay (4)
overlay.unmount(overlayIds[3]);
await waitFor(() => {
expect(screen.queryByTestId('overlay-2')).not.toBeInTheDocument();
expect(screen.queryByTestId('overlay-4')).not.toBeInTheDocument();
expect(screen.getByTestId('overlay-1')).toBeVisible();
expect(screen.getByTestId('overlay-3')).toBeVisible();
});

// Remove overlay 3
overlay.unmount(overlayIds[2]);
await waitFor(() => {
expect(screen.queryByTestId('overlay-2')).not.toBeInTheDocument();
expect(screen.queryByTestId('overlay-3')).not.toBeInTheDocument();
expect(screen.queryByTestId('overlay-4')).not.toBeInTheDocument();
expect(screen.getByTestId('overlay-1')).toBeVisible();
});

// Remove overlay 1
overlay.unmount(overlayIds[0]);
await waitFor(() => {
expect(screen.queryByTestId(/^overlay-/)).not.toBeInTheDocument();
});
});

it('should track current overlay state correctly', async () => {
const overlayIdMap = {
first: 'overlay-content-1',
second: 'overlay-content-2',
};

function Component() {
const current = useCurrentOverlay();

useEffect(() => {
overlay.open(({ isOpen }) => isOpen && <div data-testid="overlay-1">{overlayIdMap.first}</div>, {
overlayId: overlayIdMap.first,
});
}, []);

return <div data-testid="current-overlay">{current}</div>;
}

render(<Component />, { wrapper });

// Test 1: Verify state after first overlay is opened
await waitFor(() => {
expect(screen.getByTestId('overlay-1')).toBeVisible();
expect(screen.getByTestId('current-overlay')).toHaveTextContent(overlayIdMap.first);
});

// Test 2: Verify state is cleared after closing first overlay
overlay.close(overlayIdMap.first);
await waitFor(() => {
expect(screen.queryByTestId('overlay-1')).not.toBeInTheDocument();
expect(screen.getByTestId('current-overlay')).toHaveTextContent('');
});

// Test 3: Verify state after second overlay is opened
overlay.open(({ isOpen }) => isOpen && <div data-testid="overlay-2">{overlayIdMap.second}</div>, {
overlayId: overlayIdMap.second,
});
await waitFor(() => {
expect(screen.getByTestId('overlay-2')).toBeVisible();
expect(screen.getByTestId('current-overlay')).toHaveTextContent(overlayIdMap.second);
});

// Test 4: Verify state is cleared after unmounting second overlay
overlay.unmount(overlayIdMap.second);
await waitFor(() => {
expect(screen.queryByTestId('overlay-2')).not.toBeInTheDocument();
expect(screen.getByTestId('current-overlay')).toHaveTextContent('');
});
});
});
0