toss / overlay-kit Goto Github PK
View Code? Open in Web Editor NEWThe simplest and most intuitive way to manage overlays in React.
Home Page: https://overlay-kit.slash.page
License: MIT License
The simplest and most intuitive way to manage overlays in React.
Home Page: https://overlay-kit.slash.page
License: MIT License
Current Documentation Challenges
The current documentation for the overlay-kit is scattered across multiple sources. Users must read through various documents to fully understand how to use the overlay-kit, and there are significant risks of missing important information, such as precautions and best practices.
Proposed Solution
This issue aims to consolidate and enhance the documentation to make it more intuitive and accessible. The goal is to ensure that users can understand what the overlay-kit is designed to achieve without having to look at example code. We need to present a clear, unified view of the tool’s functionality and usage guidelines in one comprehensive document.
안녕하세요! overlay 객체 공식 문서를 읽고 overlay.open(...)
콜백 함수의 open
, close
, unmount
와 같은 프로퍼티들이 어떻게 동작하는지 내부 구현을 보고 싶어서 모듈 파일을 확인해봤는데 컴파일되어 있는 결과물만 있는 것 같은데 어떻게 하면 내부 구현 코드를 확인할 수 있나요??
The overlay kit provider seems to behave differently than other libraries. Is this intended behavior?
I realized that the following actions were needed in the test code to test the operation of overlay-kit.
afterEach(() => {
overlay.unmountAll();
});
The reason this is necessary is because the OverlayProvider is separate from the elements mounted in the actual DOM.
Therefore, in most cases providing different OverlayProviders for individual tests does not make sense from the perspective of making the tests independent.
const wrapper = ({ children }: PropsWithChildren) => <OverlayProvider>{children}</OverlayProvider>;
However, libraries such as jotai can guarantee test independence by providing different providers.
import { Provider as JotaiProvider } from 'jotai'
const wrapper = ({ children }: PropsWithChildren) => <JotaiProvider>{children}</JotaiProvider>;
Personally, I think jotai's approach is closer to the provider role that users expect.
If this is the intended design, it would be good to provide documentation on how to test with overlay-kit.
If this wasn't intended, I think we need to think about how to do it.
I'm curious about your opinion,
Thank you for making a great library thanks! 🙂
This problem occurred in overlay-kit 1.3.0 version.
I ran into the following problem while working with "overlay-kit".
Components inside the "overlay.open" do not react properly when the state exists externally
export default function Page() {
const [notWorkingText, setNotWorkingText] = useState("world");
return (
<div>
<button
onClick={() => {
overlay.open(({ isOpen }) => {
return isOpen ? (
<div>
<div>{notWorkingText}</div>
<button
onClick={() => {
setNotWorkingText("hello");
}}
>
text change
</button>
</div>
) : null;
});
}}
>
not work example button
</button>
</div>
)
When I click the "Change Text" button, I think the text that appears on the screen should change from world to hello.
But this example doesn't actually work
Please let me know if I'm doing something wrong
<button
onClick={() => {
overlay.open(({ isOpen, close }) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const [workingText, setWorkingText] = useState("world");
return (
<div>
<div>{workingText}</div>
<button
onClick={() => {
setWorkingText("hello");
}}
>
text change
</button>
</div>
);
});
}}
>
work example
</button>
This example works perfectly. I think it probably responds appropriately to state changes because it holds the state inside the component.
If this isn't a bug, I'm curious how this use case can be handled.
I've posted a code snippet that reproduces this issue
Thank you for your efforts
If there is no OverlayKitProvider
in the parent tree and overlay functions are attempted to be used in child trees, an error message should be displayed. This message should inform users that the OverlayKitProvider
is missing and that overlay functions cannot be used without it.
Currently, there is no build or runtime error when OverlayKitProvider
is missing.
Implement a runtime error to be triggered when OverlayKitProvider
is not present in the parent tree. This will prevent the use of overlay-kit functions in child trees, similar to how Recoil
handles such cases.
Add a changeset for versioning.
It's not a problem, but I cautiously suggest that it would be better if overlay-kit had this function.
Could you please consider adding explicit support for Promise to the overlay kit?
I'm opening an issue because there's a feature I'd like to suggest.
overlay-kit documentation provides some nice examples of using overlay-kit with Promise.
It works well, but as you can see in the documentation examples, creating Promise, creating callbacks, and opening overlays feels like a lot of boilerplate
I think it would be good to have a feature that reduces boilerplate and easily supports Promise use cases.
The current usage is as follows:
const result = await new Promise<boolean>(resolve => {
overlay.open(({ isOpen, close }) => {
return (
<AlertDialog
open={isOpen}
title="Are you sure?"
onConfirm={() => {
resolve(true);
close();
}}
onCancel={() => {
resolve(false);
close();
}}
/>
)
})
});
The rough API I think of is as follows.
const result = await overlay.promise(({ isOpen,close, resolve}) => (
<AlertDialog
open={isOpen}
title="Are you sure?"
onConfirm={() => {
resolve(true);
close();
}}/>
onCancel={() => {
resolve(false)
close()
}}
))
The nice thing about this implementation is that logic like creating new Promise is abstracted away, reducing boilerplate.
I tested a simple piece of code that implements this use case and found that it worked great.
If this suggestion looks good, I'd like to work on it.
The experience and mental model of using this library are excellent. Thank you for creating a good library.
Please consider my offer. Bye.
Changing the management of the overlays state to be outside of React.
Currently, the overlays state is managed within React, necessitating a hook to access its value.
Ideally, overlay-kit should allow access to the state without requiring a hook. Thus, moving the state management outside of React.
Consequently, the current event-based state management method will be changed to useSyncExternalStore.
Relate: #19
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
.
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. 🧐
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.
We are planning to add support for both React 16.8 and React 17 versions in this project. This upcoming update will ensure compatibility and provide flexibility for developers using different versions of React in their projects.
안녕하세요. 좋은 라이브러리 만들어주셔서 감사합니다 :)
기존 @toss/use-overlay도 잘 사용하고 있었어서 이번에 업데이트 된 overlay-kit도 바로 적용해보았는데요.
마이그레이션을 하다 보니 이런 케이스가 있더라고요.
const overlay = useOverlay()
// 중략...
return {
openModal: openFooModal,
closeModal: overlay.exit,
};
위 예시 케이스는 openFooModal
에서 구체적인 비즈니스 로직을 포함한 모달 관리 로직을 구현하고, 그 모달 내부에서 돌려 받은 콜백에서 컨트롤이 끝나는게 아니라 외부로 해당 모달의 id가 담긴 overlay.exit
을 노출시켜야 하는 경우였습니다.
하지만 이번 변경으로 인해 overlay가 훅으로 부터 분리되면서 close, unmount 시 id를 맥락으로 가지고 있지 못하게 되었고, 별도로 useRef 등 수단에 id를 담아주는 구현을 추가해야 했습니다.
물론 의도하신 디자인일수도 있으나 기존 라이브러리로부터의 마이그레이션을 돕는 관점에서든 id 값에 대한 캡슐화 관점에서든 useOverlay를 제공해주시면 조금 더 편하게 활용할 수 있는 라이브러리가 되지 않을까 싶어 이슈를 올려봅니다.
아래는 제가 간단히 구현해본 버전입니다
import { useCallback, useRef } from 'react';
import { overlay } from 'overlay-kit';
// OverlayControllerComponent가 내부 타입으로만 선언되어 있어 overlay의 타입을 뽑아왔습니다.
type OverlayControllerComponent = Parameters<(typeof overlay)['open']>[number];
const useOverlay = () => {
const id = useRef<string | null>(null);
const open = useCallback((controller: OverlayControllerComponent) => {
id.current = overlay.open(controller);
return overlay.close.bind(null, id.current);
}, []);
const close = useCallback(() => {
if (id.current !== null) {
overlay.close(id.current);
id.current = null;
}
}, []);
const unmount = useCallback(() => {
if (id.current !== null) {
overlay.unmount(id.current);
id.current = null;
}
}, []);
return {
...overlay,
open,
close,
unmount,
};
};
export default useOverlay;
If using useOverlay
hook of slash, we couldn't handle this use case:
function Component() {
const overlay = useOverlay()
// HOW could I access overlay state such as open/close state?
return (
<button onClick={() => overlay.open({ isOpen, close } => <Dialog isOpen={isOpen} close={close} />)}>open overlay</button>
)
}
overlay-kit
does not provide useOverlay
hook, but it has the same interface as overlay
object returned by useOverlay
. That is, overlay
state is only known inside Dialog
component, and there is no way to know the state outside the callback function of overlay.open
function. This can cause the following problem:
function Component() {
return (
<button onClick={() => overlay.open({ isOpen, close } => <Dialog isOpen={isOpen} close={close} />)}>open overlay</button>
{/** UI according to overlay state but.. I couldn't do that */}
)
}
There are cases where you need to change the UI, not the Dialog
, depending on overlay state(open/close). I don't think this use case is uncommon. I've been in this situation too.
Has this use case ever been required in Toss products? If so, can't we use the API provided by overlay-kit
?
I'd like to suggest migrating the current "overlay-kit"'s testing methods to use userEvent. Would you please consider it?
By using userEvent, you will be able to write test code in a form that is more similar to the user's actual use cases.
The more your tests resemble the way your software is used, the more confidence they can give you.
All test cases currently used in "overlay-kit" can be migrated to userEvent without any problems.
If this proposal seems like a necessary but tedious task,
I am ready to work on it.
Could you please consider this suggestion?
Thank you always for your dedication.
While working with the overlay system, I noticed an unexpected behavior where overlays do not maintain their state upon being reopened after a close action, despite not being removed from the DOM.
This behavior deviates from the expected functionality where overlays should retain their state between close and reopen actions when not explicitly unmounted.
When an overlay is closed (without unmounting) and then reopened, it should retain its previous state and appear as it was before being closed. This expected behavior is demonstrated in the attached video.
Currently, overlays reset to their initial state when reopened after being closed, regardless of whether they were removed from the DOM. This issue is evident as the component appears to reinitialize, losing any state it previously held. The problem is illustrated in the attached video.
overlay.open()
.This issue may be stemming from an inadvertent update of overlayData
within the overlayReducer
during the reopening of an overlay. It is crucial to ensure that the overlayData
remains unchanged if an overlay is closed but not unmounted, to maintain its state across open/close cycles.
This issue was discovered while implementing a feature intended to reuse overlays efficiently. The expected and current behaviors have been documented through video examples to aid in understanding the issue clearly.
Originally posted by @jungpaeng in #55 (comment)
As I looked at the code, I had the following questions.
Neither overlay.close()
nor close
will change the current
.
When I saw the name current
, I thought it was related to the overlay
that is currently open
, but it's not.
Currently, current
only changes in relation to unmount
.
Is this the intended behavior?
I ran the following tests.
import { act, render, screen } from '@testing-library/react';
import { useEffect, type PropsWithChildren } from 'react';
import { afterEach, describe, expect, it } from 'vitest';
import { OverlayProvider, useCurrentOverlay } from './context';
import { overlay } from './event';
afterEach(() => {
overlay.unmountAll();
});
describe('overlay object', () => {
it('overlay.close current test', async () => {
const wrapper = ({ children }: PropsWithChildren) => <OverlayProvider>{children}</OverlayProvider>;
const testContent1 = 'context-modal-test-content-1';
const testContent2 = 'context-modal-test-content-2';
const closeCurrent = 'close-current';
let current: string | null = null;
const Component = () => {
current = useCurrentOverlay();
useEffect(() => {
overlay.open(
({ isOpen }) => {
return <>{isOpen && <p>{testContent1}</p>}</>;
},
{ overlayId: 'overlayId1' }
);
overlay.open(
({ isOpen, close }) => {
return <>{isOpen && <p onClick={close}>{testContent2}</p>}</>;
},
{ overlayId: 'overlayId2' }
);
}, []);
return <div onClick={() => overlay.close(current!)}>{closeCurrent}</div>;
};
const renderComponent = render(<Component />, { wrapper });
const testContentElement2 = await renderComponent.findByText(testContent2);
const closeCurrenteElement = await renderComponent.findByText(closeCurrent);
act(() => {
testContentElement2.click(); // I expect `current` to change overlayId1.
closeCurrenteElement.click(); // I expect `testContent1` to be closed.
});
expect(screen.queryByText(testContent1)).not.toBeInTheDocument();
expect(screen.queryByText(testContent2)).not.toBeInTheDocument();
expect(current).toBe(null);
});
});
The context-modal-test-content-1
still exists.
The current
returns overlayId2
, not null
.
After merging this PR #33, I observed that it is possible to happen the memory leak.
The repro on this issue. You can check the memory profiling.
https://github.com/ojj1123/overlay-kit-memory-leak
The points of the repro is below:
react-router-dom
)unmount
callback and didn't set the custom overlayId
.Every calling overlay.open()
w/o the custom overlayId
, the new random id is generated by randomId()
and then the overlayItem
with that id
is stored to overlayData
. Even if unmounting OverlayProvider
, the overlayItem
s are not GC'd.
I think that the problem is not to call unmount
. In other word, the users of overlay-kit
need to use unmount
callback for preventing memory leak. And as SPA, there are no refresh or reload so the overlay state is not refreshed.
When close is executed, the overlay changes isOpen to false, making it disappear from the screen, but it remains in memory and the DOM. Additionally, I assumed that executing close without unmount means the user is aware of this and intentionally keeps the overlay in memory. As a result, close does not modify current.
#44 (comment)
You said the users intent to remain the overlays in memory, but without the custom id, the same overlay is never re-used since every calling overlay.open
, the new overlay is generated. So I'm wondering what's the advantage of storing the overlay states.
I came up with some workaround, but I didn't try that yet.
As useOverlay of slash, it has clean-up function to unmount overlay. Like this, overlay-kit
need to clean up the overlay state.
If you want to cache the state, we can use WeakMap
. So you change overlayData
from object to WeakMap
setting the keys to overlayItem
:
export type OverlayData = {
current: OverlayId | null;
overlayOrderList: OverlayId[];
overlayData: WeakMap<OverlayItem, any>;
};
If no refer to OverlayItem
, that overlayItem
will be GC'd. But as the situation below, maybe that are not GC'd:
OverlayProvider
, all of overlayItem
are already referred.overlayData
using useOverlayData
, it can't be GC'd.As #33, you changed the management of the overlay state from React's internal state(useReducer
) to external state(useSyncExternalStore
). So even if OverlayProvider
is unmounted, the overlay state is not cleared. Therefore, if you manage it as React's internal state, you will be able to initialize the state when OverlayProvider
is unmounted.
You can just let the users know you need to call unmount
in the Document. But if the user don't take care of it, the memory leak occurs. So I think that it is good for this library to provider unmounting overlay properly.
Making the repro, I found other bug:
If unmounting OverlayProvider
and mounting OverlayProvider
again, all of isOpen
set to true
. This is because when mounting OverlayProvider
, effect function is called in ContentOverlayController
. You can see this bug in my repro.
overlay-kit/packages/src/context/provider.tsx
Lines 64 to 66 in 7ca5087
It's not higher. ^18 only cover major version 18. not 19 so In my opinion, this should be updated
Originally posted by @manudeli in #23 (comment)
When specifying custom-id
separately on overlay.open
, multiple occurrences of that event will result in an error.
Example code
<button
onClick={() => {
overlay.open(
({ isOpen, close, unmount }) => {
return (
<CenterModal isOpen={isOpen} onExit={unmount}>
<div>
<p>MODAL CONTENT</p>
<button onClick={close}>close modal</button>
</div>
</CenterModal>
);
},
{ overlayId: 'overlayid-1' }
);
}}
>
open center modal
</button>
Demo
This appears to be caused by the same overlayId
already existing in the overlayOrderList
.
I thought of adding the following code in the ADD
action.
If the overlayId
already exists, return the existing state
.
reducer.ts
case 'ADD': {
if (state.overlayOrderList.includes(action.overlay.id)) {
return state;
}
return {
current: action.overlay.id,
overlayOrderList: [...state.overlayOrderList, action.overlay.id],
overlayData: {
...state.overlayData,
[action.overlay.id]: action.overlay,
},
};
}
Expected behavior
If you think the suggestion is appropriate, would you mind if I add a feature fix and test code? 🧐
overlay-kit/packages/src/utils/create-use-external-events.ts
Lines 12 to 13 in f377f72
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.