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.

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

Add the ability to block navigation when forms have not been saved #11831

Merged
merged 14 commits into from
Apr 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion airbyte-webapp/.eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
"group": "internal"
},
{
"pattern": "+(config|core|hooks|locales|packages|pages|services|utils|views){/**,}",
"pattern": "+(config|core|hooks|locales|packages|pages|services|types|utils|views){/**,}",
"group": "internal",
"position": "after"
}
Expand Down
8 changes: 7 additions & 1 deletion airbyte-webapp/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import { FeatureService } from "hooks/services/Feature";
import { ServicesProvider } from "core/servicesProvider";
import { ApiServices } from "core/ApiServices";
import { StoreProvider } from "views/common/StoreProvider";
import { ConfirmationModalService } from "hooks/services/ConfirmationModal";
import { FormChangeTrackerService } from "hooks/services/FormChangeTracker";

import en from "./locales/en.json";
import GlobalStyle from "./global-styles";
Expand Down Expand Up @@ -53,7 +55,11 @@ const Services: React.FC = ({ children }) => (
<WorkspaceServiceProvider>
<FeatureService>
<NotificationService>
<ApiServices>{children}</ApiServices>
<ConfirmationModalService>
<FormChangeTrackerService>
<ApiServices>{children}</ApiServices>
</FormChangeTrackerService>
</ConfirmationModalService>
</NotificationService>
</FeatureService>
</WorkspaceServiceProvider>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import React from "react";
import styled from "styled-components";
import { FormattedMessage } from "react-intl";

import Modal from "components/Modal";
import { Button } from "components/base/Button";

const Content = styled.div`
width: 585px;
font-size: 14px;
line-height: 28px;
padding: 25px;
white-space: pre-line;
`;

const ButtonContent = styled.div`
margin-top: 26px;
display: flex;
justify-content: flex-end;
`;

const ButtonWithMargin = styled(Button)`
margin-right: 12px;
`;

export interface ConfirmationModalProps {
onClose: () => void;
title: string;
text: string;
submitButtonText: string;
onSubmit: () => void;
}

export const ConfirmationModal: React.FC<ConfirmationModalProps> = ({
onClose,
title,
text,
onSubmit,
submitButtonText,
}) => (
<Modal onClose={onClose} title={<FormattedMessage id={title} />}>
<Content>
<FormattedMessage id={text} />
<ButtonContent>
<ButtonWithMargin onClick={onClose} type="button" secondary>
<FormattedMessage id="form.cancel" />
</ButtonWithMargin>
<Button type="button" danger onClick={onSubmit}>
<FormattedMessage id={submitButtonText} />
</Button>
</ButtonContent>
</Content>
</Modal>
);
1 change: 1 addition & 0 deletions airbyte-webapp/src/components/ConfirmationModal/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { ConfirmationModal } from "./ConfirmationModal";
Copy link
Contributor

Choose a reason for hiding this comment

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

Tim has specifically called out to stop using this pattern. We should be exporting thing and then import { thing } where needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for the confusion I apparently created there. I don't think this is an anti pattern. I was mainly calling out default imports, but named re-exporting from a file and having index.ts files, would be my idea how we should export.

Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,11 @@ const CreateConnectionContent: React.FC<IProps> = ({
},
});

if (afterSubmitConnection) {
afterSubmitConnection(connection);
}
return {
onSubmitComplete: () => {
afterSubmitConnection?.(connection);
},
};
};

const onSelectFrequency = (item: IDataItem | null) => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { useEffect } from "react";
import { usePrevious } from "react-use";

import { useFormChangeTrackerService, useUniqueFormId } from "hooks/services/FormChangeTracker";

interface Props {
changed: boolean;
formId?: string;
}

export const FormChangeTracker: React.FC<Props> = ({ changed, formId }) => {
const id = useUniqueFormId(formId);
const prevChanged = usePrevious(changed);

const { trackFormChange } = useFormChangeTrackerService();

useEffect(() => {
if (changed !== prevChanged) {
trackFormChange(id, changed);
}
}, [id, changed, trackFormChange, prevChanged]);

return null;
};
1 change: 1 addition & 0 deletions airbyte-webapp/src/components/FormChangeTracker/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { FormChangeTracker } from "./FormChangeTracker";
40 changes: 40 additions & 0 deletions airbyte-webapp/src/hooks/router/useBlocker.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import type { Blocker, History, Transition } from "history";

import { ContextType, useContext, useEffect } from "react";
import { Navigator as BaseNavigator, UNSAFE_NavigationContext as NavigationContext } from "react-router-dom";

interface Navigator extends BaseNavigator {
block: History["block"];
}

type NavigationContextWithBlock = ContextType<typeof NavigationContext> & { navigator: Navigator };

/**
* @source https://github.com/remix-run/react-router/commit/256cad70d3fd4500b1abcfea66f3ee622fb90874
*/
export const useBlocker = (blocker: Blocker, block = true) => {
const { navigator } = useContext(NavigationContext) as NavigationContextWithBlock;
krishnaglick marked this conversation as resolved.
Show resolved Hide resolved

useEffect(() => {
if (!block) {
return;
}

const unblock = navigator.block((tx: Transition) => {
const autoUnblockingTx = {
...tx,
retry() {
// Automatically unblock the transition so it can play all the way
// through before retrying it. TODO: Figure out how to re-enable
// this block if the transition is cancelled for some reason.
unblock();
tx.retry();
},
};

blocker(autoUnblockingTx);
});

return unblock;
}, [navigator, blocker, block]);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import React, { useContext, useEffect, useMemo } from "react";

import { ConfirmationModal } from "components/ConfirmationModal";

import useTypesafeReducer from "hooks/useTypesafeReducer";

import { ConfirmationModalOptions, ConfirmationModalServiceApi, ConfirmationModalState } from "./types";
import { actions, initialState, confirmationModalServiceReducer } from "./reducer";

const ConfirmationModalServiceContext = React.createContext<ConfirmationModalServiceApi | undefined>(undefined);

export const useConfirmationModalService: (confirmationModal?: ConfirmationModalOptions) => {
openConfirmationModal: (confirmationModal: ConfirmationModalOptions) => void;
closeConfirmationModal: () => void;
} = (confirmationModal) => {
const confirmationModalService = useContext(ConfirmationModalServiceContext);
if (!confirmationModalService) {
throw new Error("useConfirmationModalService must be used within a ConfirmationModalService.");
}

useEffect(() => {
if (confirmationModal) {
confirmationModalService.openConfirmationModal(confirmationModal);
}
return () => {
if (confirmationModal) {
confirmationModalService.closeConfirmationModal();
}
};
}, [confirmationModal, confirmationModalService]);

return useMemo(
() => ({
openConfirmationModal: confirmationModalService.openConfirmationModal,
closeConfirmationModal: confirmationModalService.closeConfirmationModal,
}),
[confirmationModalService]
);
};

export const ConfirmationModalService = ({ children }: { children: React.ReactNode }) => {
const [state, { openConfirmationModal, closeConfirmationModal }] = useTypesafeReducer<
ConfirmationModalState,
typeof actions
>(confirmationModalServiceReducer, initialState, actions);

const confirmationModalService: ConfirmationModalServiceApi = useMemo(
() => ({
openConfirmationModal,
closeConfirmationModal,
}),
[closeConfirmationModal, openConfirmationModal]
);

return (
<>
<ConfirmationModalServiceContext.Provider value={confirmationModalService}>
{children}
</ConfirmationModalServiceContext.Provider>
{state.isOpen && state.confirmationModal ? (
<ConfirmationModal
onClose={closeConfirmationModal}
title={state.confirmationModal.title}
text={state.confirmationModal.text}
onSubmit={state.confirmationModal.onSubmit}
submitButtonText={state.confirmationModal.submitButtonText}
/>
) : null}
</>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "./ConfirmationModalService";
Copy link
Contributor

Choose a reason for hiding this comment

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

See other export comments.

31 changes: 31 additions & 0 deletions airbyte-webapp/src/hooks/services/ConfirmationModal/reducer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { ActionType, createAction, createReducer } from "typesafe-actions";

import { ConfirmationModalOptions, ConfirmationModalState } from "./types";

export const actions = {
openConfirmationModal: createAction("OPEN_CONFIRMATION_MODAL")<ConfirmationModalOptions>(),
closeConfirmationModal: createAction("CLOSE_CONFIRMATION_MODAL")(),
};

type Actions = ActionType<typeof actions>;

export const initialState: ConfirmationModalState = {
isOpen: false,
confirmationModal: null,
};

export const confirmationModalServiceReducer = createReducer<ConfirmationModalState, Actions>(initialState)
.handleAction(actions.openConfirmationModal, (state, action): ConfirmationModalState => {
return {
...state,
isOpen: true,
confirmationModal: action.payload,
};
})
.handleAction(actions.closeConfirmationModal, (state): ConfirmationModalState => {
return {
...state,
isOpen: false,
confirmationModal: null,
};
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm misunderstanding something, it seems like this could be simplified down without the use of a reducer since it's two states.
You're either opening it and passing it the modal options, or closing it and nulling those options out no?

13 changes: 13 additions & 0 deletions airbyte-webapp/src/hooks/services/ConfirmationModal/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { ConfirmationModalProps } from "components/ConfirmationModal/ConfirmationModal";

export type ConfirmationModalOptions = Omit<ConfirmationModalProps, "onClose">;

export interface ConfirmationModalServiceApi {
openConfirmationModal: (confirmationModal: ConfirmationModalOptions) => void;
closeConfirmationModal: () => void;
}

export interface ConfirmationModalState {
isOpen: boolean;
confirmationModal: ConfirmationModalOptions | null;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import type { Transition } from "history";

import React, { useCallback, useMemo } from "react";

import { useBlocker } from "hooks/router/useBlocker";

import { useConfirmationModalService } from "../ConfirmationModal";
import { useChangedFormsById } from "./hooks";

export const FormChangeTrackerService: React.FC = ({ children }) => {
const [changedFormsById, setChangedFormsById] = useChangedFormsById();
const { openConfirmationModal, closeConfirmationModal } = useConfirmationModalService();

const blocker = useCallback(
(tx: Transition) => {
openConfirmationModal({
title: "form.discardChanges",
text: "form.discardChangesConfirmation",
submitButtonText: "form.discardChanges",
onSubmit: () => {
setChangedFormsById({});
closeConfirmationModal();
tx.retry();
},
});
},
[closeConfirmationModal, openConfirmationModal, setChangedFormsById]
);

const formsChanged = useMemo(
() => Object.values(changedFormsById ?? {}).some((formChanged) => formChanged),
[changedFormsById]
);

useBlocker(blocker, formsChanged);

return <>{children}</>;
};
39 changes: 39 additions & 0 deletions airbyte-webapp/src/hooks/services/FormChangeTracker/hooks.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import { useCallback, useMemo } from "react";
import { createGlobalState } from "react-use";
import { uniqueId } from "lodash";

import { FormChangeTrackerServiceApi } from "./types";

export const useChangedFormsById = createGlobalState<Record<string, boolean>>({});

export const useUniqueFormId = (formId?: string) => useMemo(() => formId ?? uniqueId("form_"), [formId]);

export const useFormChangeTrackerService = (): FormChangeTrackerServiceApi => {
const [changedFormsById, setChangedFormsById] = useChangedFormsById();

const clearAllFormChanges = useCallback(() => {
setChangedFormsById({});
}, [setChangedFormsById]);

const clearFormChange = useCallback(
(id: string) => {
setChangedFormsById({ ...changedFormsById, [id]: false });
},
[changedFormsById, setChangedFormsById]
);

const trackFormChange = useCallback(
(id: string, changed: boolean) => {
if (Boolean(changedFormsById?.[id]) !== changed) {
setChangedFormsById({ ...changedFormsById, [id]: changed });
}
},
[changedFormsById, setChangedFormsById]
);

return {
trackFormChange,
clearFormChange,
clearAllFormChanges,
};
};
2 changes: 2 additions & 0 deletions airbyte-webapp/src/hooks/services/FormChangeTracker/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from "./FormChangeTrackerService";
export * from "./hooks";
Copy link
Contributor

Choose a reason for hiding this comment

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

See other export comments

5 changes: 5 additions & 0 deletions airbyte-webapp/src/hooks/services/FormChangeTracker/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export interface FormChangeTrackerServiceApi {
trackFormChange: (id: string, changed: boolean) => void;
clearFormChange: (id: string) => void;
clearAllFormChanges: () => void;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love to get @timroes opinion on this pattern.
I'm not the biggest fan and would prefer to place in the component or service, but content to fall either way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have anything against this pattern. No strong feelings in either direction. Advantage of pulling it into a separate type file like this (as long as we need to export the type), is that we'd potentially prevent some cyclic dependency further down the road, in which case we'd then need to extract it into a separate type file.

1 change: 1 addition & 0 deletions airbyte-webapp/src/locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
"form.sourceRetest": "Retest source",
"form.destinationRetest": "Retest destination",
"form.discardChanges": "Discard changes",
"form.discardChangesConfirmation": "There are unsaved changes. Are you sure you want to discard your changes?",
"form.every": "Every {value}",
"form.testingConnection": "Testing connection...",
"form.successTests": "All connection tests passed!",
Expand Down
Loading