[Question] Is it correct that overlay.current exists even if I unmount, unmountAll?

[jgjgill]

Hi, I'm learning a lot from your code. I have a question.

I think when I use unmountAll, the overlays value is to be initial overlays value. But now overlays.current is state.current.

Test

I proceeded with the test as follows.

store.ts

export function dispatchOverlay(action: OverlayReducerAction) {
  overlays = overlayReducer(overlays, action);
  console.log(overlays); // I add console

  emitChangeListener();
}

temp test code

it('overlay unmount, unmountAll test', async () => {
  const wrapper = ({ children }: PropsWithChildren) => <OverlayProvider>{children}</OverlayProvider>;

  const testContent1 = 'context-modal-test-content-1';

  const Component = () => {
    useEffect(() => {
      const overlayId = overlay.open(() => {
        return <p>{testContent1}</p>;
      });

      overlay.unmountAll(); // I expect { current: null, overlayOrderList: [], overlayData: {} }
      overlay.unmount(overlayId); // I expect { current: null, overlayOrderList: [], overlayData: {} }
    }, []);

    return <div>Empty</div>;
  };

  render(<Component />, { wrapper });
  // How can I check overlays.current here? Is it right to test store.ts 🤔?

I checked the following console. 🧐

Suggestion

In my opinion, the following code should be changed

• state.current => null

reducer.ts

    case 'REMOVE': {
      ...
      return {
        current: remainingOverlays.at(-1) ?? null, // I also think empty strings could be in the `overlayId`.
        overlayOrderList: remainingOverlays,
        overlayData: copiedOverlayData,
      };
    }

    case 'REMOVE_ALL': {
      return { current: null, overlayOrderList: [], overlayData: {} };
    }

Thanks for the great library.

[jungpaeng]

You’re correct in your observation.

To explain the background of this implementation: initially, during the PoC (Proof of Concept) phase of this library, the add and open steps of the reducer were combined into a single step, and similarly, the close and unmount steps were combined into one.

As a result, calling closeAll would trigger unmountAll immediately afterwards. In very specific scenarios, we noticed an issue where the animation would break. To temporarily fix this, we found that setting the current value helped resolve the issue.

To properly address this animation issue, we later decided to separate the add and open steps, as well as the close and unmount steps.

In other words, now that the close and unmount steps are separated, there’s no longer a need for the current value to point to the last modal. We will update this to point to null instead.

[jungpaeng]

The part you mentioned about overlayId potentially being an empty string was not intentional. It could lead to bugs.

The possibility of overlayId being an empty string could arise when a custom ID is specified. We need to prevent setting an empty string as a custom ID.

[jgjgill]

Oh, Thank you for your detailed explanation! I now have a better understanding of the library. 🙇‍♂️