feat(core): added useShowModal that passes the application to the context in showModal

[ranihorev]

Problem

Some components use the ApplicationContext inside of a modal, however, it's not part of the react tree and therefore can't use the context.

Solution

Added a new parameter - application - to showModal and created a new hook that passes the application from the context to showModal

[caseyhebebrand]

It feels like the hook is adding a lot of code to do something simple. IMO it is more readable to fetch the application separately if it's needed, then pass the app context as a prop when invoking showModal. With the suggested approach (below), there is a consistent way to use showModal with or without the app prop, and the component will delegate which state is rendered based on the props.

Suggested change

	const appName = useApplicationContextSafe().name;
	const { data, refetch } = useFetchApplicationManagementStatusQuery({ variables: { appName } });
	const [toggleManagement] = useToggleManagementMutation();
	const showModal = useShowModal();
	const appContext = useApplicationContextSafe();
	const { data, refetch } = useFetchApplicationManagementStatusQuery({ variables: { appName: app.name } });
	const [toggleManagement] = useToggleManagementMutation();
	// later...
	showModal(ResumeManagementModal, props, { maxWidth: MODAL_MAX_WIDTH }, appContext);

[ranihorev]

That was my original implementation but I thought that it could be useful to add this abstraction so the consumer of the modal won't have deal with that. Does make it sense?

[caseyhebebrand]

Can we call it useShowModalWithAppContext?

[christopherthielen]

I think this problem would be better solved using react portals in showModal

[ranihorev]

I agree, I was a bit surprised that we weren't using it but didn't want to update the current implementation, because it might break some of the existing usage unintentionally. Maybe I should introduce a new hook. What do you think?

…

On Tue, Jun 22, 2021 at 06:07 Chris Thielen ***@***.***> wrote: I think this problem would be better solved using react portals in showModal — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9363 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEC2OROQD5AR27QWDCV27JTTT746FANCNFSM47AI5WYA> .

[christopherthielen]

can you explain why adding a portal might break current uses? TBH I'm a bit surprised we arent already doing this in showModal too.

[ranihorev]

When using portal event bubbling and contexts work differently (they work 🙃). While it probably won't affect the existing modal, I can't say that for sure.

…

On Tue, Jun 22, 2021 at 22:51 Chris Thielen ***@***.***> wrote: can you explain why adding a portal might break current uses? TBH I'm a bit surprised we arent already doing this in showModal. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9363 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEC2ORJVG4HL6ZBZWHGQAUTTUDST3ANCNFSM47AI5WYA> .

[ranihorev]

When using portal event bubbling and contexts work differently (they work 🙃). While it probably won't affect the existing modal, I can't say that for sure.

The current workflow won't fit portal afaik. It has to be rendered as a react component and not via a function call (i.e. showModal). We might be able to add a separate workflow for that