Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion app/scripts/modules/core/src/managed/config/Configuration.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import classnames from 'classnames';
import React from 'react';

import { Illustration } from '@spinnaker/presentation';
import { showModal, useApplicationContextSafe } from 'core/presentation';
import { useApplicationContextSafe, useShowModal } from 'core/presentation';
import { Spinner } from 'core/widgets';

import { ApplicationQueryError } from '../ApplicationQueryError';
Expand Down Expand Up @@ -42,6 +42,7 @@ const ManagementToggle = () => {
const logEvent = useLogEvent('Management');
const { data, error, loading, refetch } = useFetchApplicationManagementStatusQuery({ variables: { appName } });
const [toggleManagement, { loading: mutationInFlight }] = useToggleManagementMutation();
const showModal = useShowModal();

const onShowToggleManagementModal = React.useCallback((shouldPause: boolean) => {
logEvent({ action: 'OpenModal', data: { shouldPause } });
Expand Down
7 changes: 5 additions & 2 deletions app/scripts/modules/core/src/managed/config/ManagementWarning.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
import React from 'react';

import { showModal } from 'core/presentation';
import { useApplicationContextSafe, useShowModal } from 'core/presentation';

import { ResumeManagementModal } from './Configuration';
import { useFetchApplicationManagementStatusQuery, useToggleManagementMutation } from '../graphql/graphql-sdk';
import { MODAL_MAX_WIDTH } from '../utils/defaults';

import './ManagementWarning.less';

export const ManagementWarning = ({ appName }: { appName: string }) => {
export const ManagementWarning = () => {
const appName = useApplicationContextSafe().name;
const { data, refetch } = useFetchApplicationManagementStatusQuery({ variables: { appName } });
const [toggleManagement] = useToggleManagementMutation();
const showModal = useShowModal();
Comment on lines +12 to +15
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call it useShowModalWithAppContext?


const onClick = React.useCallback(() => {
showModal(
Expand Down
2 changes: 1 addition & 1 deletion app/scripts/modules/core/src/managed/overview/EnvironmentsOverview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export const EnvironmentsOverview = () => {
} else {
content = (
<>
<ManagementWarning appName={app.name} />
<ManagementWarning />
{environments.length ? (
<>
<EnvironmentsRender {...regularEnvironmentsProps}>
Expand Down
6 changes: 5 additions & 1 deletion app/scripts/modules/core/src/managed/overview/artifact/hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { showModal } from 'core/presentation';
import { useShowModal } from 'core/presentation';

import { MarkAsBadActionModal, MarkAsGoodActionModal, PinActionModal, UnpinActionModal } from './ArtifactActionModal';
import {
Expand All @@ -23,6 +23,7 @@ export const useUnpinVersion = ({ version, ...payload }: ActionBasePayload, moda
variables: { payload: payload },
refetchQueries: [{ query: FetchPinnedVersionsDocument, variables: { appName: payload.application } }],
});
const showModal = useShowModal();

return () => {
showModal(
Expand All @@ -46,6 +47,7 @@ export const usePinVersion = (payload: ActionBasePayload, modalTitle: string) =>
const [onPin] = usePinVersionMutation({
refetchQueries: [{ query: FetchPinnedVersionsDocument, variables: { appName: payload.application } }],
});
const showModal = useShowModal();

return () => {
showModal(
Expand All @@ -71,6 +73,7 @@ export const useMarkVersionAsBad = (payload: ActionBasePayload, modalTitle: stri
{ query: FetchVersionDocument, variables: { appName: payload.application, versions: [payload.version] } },
],
});
const showModal = useShowModal();

return () => {
showModal(
Expand All @@ -96,6 +99,7 @@ export const useMarkVersionAsGood = (payload: ActionBasePayload, modalTitle: str
{ query: FetchVersionDocument, variables: { appName: payload.application, versions: [payload.version] } },
],
});
const showModal = useShowModal();

return () => {
showModal(
Expand Down
2 changes: 1 addition & 1 deletion app/scripts/modules/core/src/managed/versionsHistory/VersionsHistory.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ export const VersionsHistory = () => {

return (
<main className="VersionsHistory">
<ManagementWarning appName={app.name} />
<ManagementWarning />
{groupedVersions.map((group) => {
return <SingleVersion key={group.key} versionData={group} pinnedVersions={pinnedVersions} />;
})}
Expand Down
1 change: 1 addition & 0 deletions app/scripts/modules/core/src/presentation/hooks/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@ export * from './useObservable.hook';
export * from './useObservableValue.hook';
export * from './usePollingData.hook';
export * from './usePrevious.hook';
export * from './useShowModal.hook';
19 changes: 19 additions & 0 deletions app/scripts/modules/core/src/presentation/hooks/useShowModal.hook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import React from 'react';
import { IModalProps, showModal } from '../modal';
import { IModalComponentProps } from '../modal/showModal';
import { useApplicationContext } from './useApplicationContext.hook';

export const useShowModal = () => {
const app = useApplicationContext();
const showModalWithApp = React.useCallback(
<P, C = any, D = any>(
ModalComponent: React.ComponentType<P & IModalComponentProps<C, D>>,
componentProps?: P,
modalProps?: Omit<IModalProps, 'isOpen' | 'onRequestClose' | 'onAfterClose' | 'children'>,
) => {
showModal(ModalComponent, componentProps, modalProps, app);
},
[showModal, app],
);
return showModalWithApp;
};
12 changes: 9 additions & 3 deletions app/scripts/modules/core/src/presentation/modal/showModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { UIRouterContextComponent } from '@uirouter/react-hybrid';
import React from 'react';
import ReactDOM from 'react-dom';

import { Application, ApplicationContextProvider } from 'core/application';

import { IModalProps, Modal } from './Modal';

/** The Modal content Component will be passed these two props */
Expand Down Expand Up @@ -51,6 +53,7 @@ export const showModal = <P, C = any, D = any>(
ModalComponent: React.ComponentType<P & IModalComponentProps<C, D>>,
componentProps?: P,
modalProps?: Omit<IModalProps, 'isOpen' | 'onRequestClose' | 'onAfterClose' | 'children'>,
application?: Application,
): Promise<IModalResult<C, D>> =>
new Promise<IModalResult<C, D>>((resolve) => {
let mountNode = document.createElement('div');
Expand Down Expand Up @@ -81,11 +84,14 @@ export const showModal = <P, C = any, D = any>(
const handleRequestClose = () => handleDismiss(null);

function render() {
const content = (
<UIRouterContextComponent>
<ModalComponent {...componentProps} dismissModal={handleDismiss} closeModal={handleClose} />
</UIRouterContextComponent>
);
ReactDOM.render(
<Modal isOpen={show} onRequestClose={handleRequestClose} onAfterClose={onAfterClose} {...modalProps}>
<UIRouterContextComponent>
<ModalComponent {...componentProps} dismissModal={handleDismiss} closeModal={handleClose} />
</UIRouterContextComponent>
{application ? <ApplicationContextProvider app={application}>{content}</ApplicationContextProvider> : content}
</Modal>,
mountNode,
);
Expand Down