Add DialogContainer #965
Add DialogContainer #965
Conversation
|
is there a way we can (or should) make this implicit to the other triggers? That is, so the user doesn't have to add a DialogContainer manually. It seems like we could do this for the user in many cases, rather than having them do it -- but maybe im missing something. Is there a case where you would ever want the DialogContainer to be somewhere else? Presumably in that case we could detect the parent context already and not automatically put one. |
|
Yeah, if there's no dialog container, dialog trigger will create one as a child, so that will be invisible to the default use case.
|
|
Yes, my point was can we also include that case (DialogTrigger within MenuTrigger or other triggers)? |
|
Oh, yeah, that makes total sense. Then there may not really be a reason to export it? |
|
|
||
| let state = useOverlayTriggerState(overlayProps); | ||
|
|
||
| // todo just pass setOverlayContent via context value |
| function DialogContainerOverlay() { | ||
| let {isDismissable, overlayContent, state, type} = useContext(DialogContainerContext); | ||
|
|
||
| let renderOverlay = () => { |
| <DialogContainerContext.Provider value={{isDismissable, state, overlayContent, addContent, setOverlayProps, type}}> | ||
| {children} | ||
| <DialogContext.Provider value={context}> | ||
| <DialogContainerOverlay /> |
There was a problem hiding this comment.
I think this might create an issue for nested dialogs if the overlay is inside the DialogContainerContext. The nested dialog trigger would consume from the context, see that a container is already there, and then reuse it. That would replace the outer dialog with the inner one rather than having both open at the same time.
Maybe you could only wrap children in the context rather than the overlay as well? Seems like the DialogContainerOverlay could be inlined into DialogContainer so it doesn't need the context. You could then use a fragment to put the context wrapping children next to the overlay rather than wrapping it.
|
|
||
| // if container exists and not being used (showing an overlay), pass trigger base, otherwise | ||
| // create container for overlay to attach to | ||
| return dialogContainerContext && !dialogContainerContext.overlayContent ? triggerBase : ( |
There was a problem hiding this comment.
Ah I see. This might fix the nested overlay issue? I think this might be a little strange though. It means that where the overlay is mounted depends on whether another overlay is already open.
| isDismissable | ||
| } = props; | ||
| // NOTE: content needs to be saved as function in order to save in useState (since it's a function as child) | ||
| let [overlayContent, setOverlayContent] = useState(() => content); |
There was a problem hiding this comment.
It's theoretically possible to have two overlays open at the same time within the same container. For example, two popovers in controlled isOpen state. The second one shouldn't go in its own container, because that would mean the location it is mounted would change depending on whether a previous overlay was open. Should this accept an array?
Closes #966
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: