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

edmundito · 2022-04-08T15:15:36Z

What

Closes #11164 Display a confirmation modal when the user navigates away from the Replication or Transformation pages with unsaved changes.

How

The component uses a global state in a hook to track each form that may need to confirm changes. It's designed to also track multiple forms on the same page but only provide one confirmation.

<MyForm>
  <FormChangesTracker changed={dirty || new} />
</MyForm>

This bridges the gap between Formik and react-router in that introduces a component to track which forms have changed so that it can display the confirmation.

react-router used to provide ways to display blockers but these were removed in 6.x (remix-run/react-router#8139). This is using hooks inspired by the changes that existed before (remix-run/react-router#8139 (comment)).

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Tests

Webapp E2E tests pass

┆Issue is synchronized with this Monday item by Unito

CLAassistant · 2022-04-08T15:15:50Z

All committers have signed the CLA.

edmundito · 2022-04-14T13:34:00Z

@airbytehq/frontend I'm pretty happy where this is at and it's ready for review.

krishnaglick · 2022-04-14T19:55:54Z

airbyte-webapp/src/components/ConfirmationModal/index.ts

@@ -0,0 +1 @@
+export { ConfirmationModal } from "./ConfirmationModal";


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

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.

krishnaglick · 2022-04-14T19:56:33Z

airbyte-webapp/src/components/FormChangeTracker/index.ts

@@ -0,0 +1,3 @@
+import FormChangeTracker from "./FormChangeTracker";
+
+export default FormChangeTracker;


See last comment.

krishnaglick · 2022-04-14T19:57:51Z

airbyte-webapp/src/hooks/router/useBlocker.ts

+  const { navigator } = useContext(NavigationContext) as NavigationContextWithBlock;
+
+  useEffect(() => {
+    if (!when) {


I feel like this could be named a little more specifically.

Maybe so. this is how it was originally designed in react-router but I'll do another pass to give it a better name.

airbyte-webapp/src/hooks/router/useBlocker.ts

krishnaglick · 2022-04-14T20:07:48Z

airbyte-webapp/src/hooks/services/ConfirmationModal/ConfirmationModalService.tsx

+        confirmationModalService.closeConfirmationModal();
+      }
+    };
+    // eslint-disable-next-line


I don't think this is needed.

krishnaglick · 2022-04-14T20:23:15Z

airbyte-webapp/src/hooks/services/FormChangeTracker/hooks.ts

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


Nitpick, this feels slightly clearer:

Suggested change

if (!!changedFormsById?.[id] !== changed) {

if (Boolean(changedFormsById?.[id]) !== changed) {

This casts the value correctly?

Yes, I specified it was a nitpick as !!changedFormsById?.[id] !== changed looks a little strange in an if-statement.
I know it functions correctly but, and perhaps this is a personal thing, I could see this reading a little confusingly as !!(changedFormsById?.[id] !== changed)

krishnaglick · 2022-04-14T20:24:25Z

airbyte-webapp/src/hooks/services/FormChangeTracker/index.ts

@@ -0,0 +1,2 @@
+export * from "./FormChangeTrackerService";
+export * from "./hooks";


See other export comments

krishnaglick · 2022-04-14T20:25:17Z

airbyte-webapp/src/hooks/services/FormChangeTracker/types.ts

+  trackFormChange: (id: string, changed: boolean) => void;
+  clearFormChange: (id: string) => void;
+  clearAllFormChanges: () => void;
+}


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.

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.

krishnaglick · 2022-04-14T20:28:33Z

airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/ReplicationView.tsx

+    if (formikHelpers) {
+      formikHelpers.resetForm({ values });
+    }


Nitpick since you do this elsewhere

Suggested change

if (formikHelpers) {

formikHelpers.resetForm({ values });

}

formikHelpers?.resetForm({ values });

krishnaglick · 2022-04-14T20:29:44Z

airbyte-webapp/src/types/formik.ts

@@ -0,0 +1,3 @@
+import { FormikConfig } from "formik";
+
+export type FormikOnSubmit<T> = FormikConfig<T>["onSubmit"];


I'd be tempted to just put this into TransformationView.tsx since that's the only place it's used, just to cut down on files. 🤷

This is pretty generic and may be needed later. I rather make it available to all now than to hide it inside a component.

* Add hooks to block and display prompt on navigation * Add components to indicate which form is blocking * Update connection replication and transformation components to block on edit * Update main page component to show prompt

useFormNavigationBlocking -> useDiscardFormChangesConfirmation Update exports from default to module

Fix forms in replications and transformations not resetting the data Add types to import sorting and add Formik helper type for onSubmit functions

Update ConnectionForm submission to handle post submission result from callback

Add hook to generate unique form id Update form tracker component to use new hook if no id is specified

timroes · 2022-04-19T09:24:52Z

airbyte-webapp/src/components/ConfirmationModal/ConfirmationModal.tsx

+        <ButtonWithMargin onClick={onClose} type="button" secondary>
+          <FormattedMessage id="form.cancel" />
+        </ButtonWithMargin>
+        <Button type="button" danger onClick={onSubmit} data-id="delete">


Do we need the data-id for something specific or just for testing purposes? If the latter, I'd suggest we name it data-testid to stay aligned with what our testing frameworks expect.

timroes · 2022-04-19T09:36:14Z

airbyte-webapp/src/hooks/services/ConfirmationModal/ConfirmationModalService.tsx

+  return {
+    openConfirmationModal: confirmationModalService.openConfirmationModal,
+    closeConfirmationModal: confirmationModalService.closeConfirmationModal,
+  };


I think we should memoize this object. While confirmationModalService.openConfirmModal and confirmationModalService.closeConfirmationModal are stable references and should not change, we're creating a new object here every time, causing the return value of useConfirmationModalService to not be stable, and thus if we're using it (without destructuring) it might cause other places to suddenly to become unstable too, e.g.:

const confirmationModal = useConfirmationModalService(); const fn = useCallback(() => { /* do something with a confirmation modal */ }, [confirmationModal]);

This will now cause the useCallback to create a new return value every render, while just changing the code slightly:

const { openConfirmationModal, closeConfirmationModal } = useConfirmationModalService(); const fn = useCallback(() => { /* do something with a confirmation modal */ }, [openConfirmationModal, closeConfirmationModal]);

This would now make the useCallback to always stay the same reference. I think that behavior might be confusing, thus I'd recommend we're also memoizing the whole object we return here from useConfirmationModalService.

timroes · 2022-04-19T09:43:06Z

airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/ReplicationView.tsx

@@ -91,6 +92,8 @@ const ReplicationView: React.FC<IProps> = ({ onAfterSaveSchema, connectionId })
    if (activeUpdatingSchemaMode) {
      setActiveUpdatingSchemaMode(false);
    }
+
+    formikHelpers?.resetForm({ values });


❓ Could you clarify why we're needing to reset the form here now?

This resetForm({ values }) updates the Formik form with the new base values so that the form is no longer marked as "dirty" during an update. The confirmation modal works based on the dirtiness of the Formik form.

Here's the sequence of events:

Form is initialized with loaded values

User edits Form, and the initial values no longer match the current values so the form is "dirty"

User submits form and request is successful

Form is reset with updated values, thus longer "dirty"

timroes · 2022-04-19T09:59:35Z

airbyte-webapp/src/components/FormChangeTracker/FormChangeTracker.tsx

+
+  const { trackFormChange } = useFormChangeTrackerService();
+
+  useLayoutEffect(() => {


❓ Is there a reason for using useLayoutEffect instead useEffect here? If so (since React also recommends using useEffect whenever possible), could we leave an inline comment why we're needing to use useLayoutEffect here?

Will change to useEffect. In earlier iterations I was testing different things and I forgot to change it back.

timroes · 2022-04-19T10:02:06Z

...e-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/TransformationView.tsx

+    const nextFormValues: typeof values = {};
+    if (values.transformations) {
+      nextFormValues.transformations = getInitialTransformations(operations);
+    }
+    if (values.normalization) {
+      nextFormValues.normalization = getInitialNormalization(operations, true);
+    }
+
+    resetForm({ values: nextFormValues });


Same question here as above: Why are we resetting this form after updating? Maybe we could leave an inline comment why this is needed.

timroes · 2022-04-19T10:05:16Z

airbyte-webapp/src/hooks/services/FormChangeTracker/hooks.ts

+export const useUniqueFormId = (formId?: string) => {
+  const location = useLocation();
+  return useMemo(
+    () => formId ?? `${location.pathname.toLowerCase().substring(1).replace(/\//gi, "_")}__${uniqueId("form_")}`,


Nitpick: do we need all that (in my personal opinion a bit harder to read) location logic? Since we're anyway generating unique ids here, is there still a use-case of prefixing them with the location path?

We don't need it, it was just easier to debug the form data this way. However, now that it's working I can remove it.

Update FormChangeTracker to useEffect over useLayoutEffect Memoize useConfirmationModalService hook Remove memoization of ConfirmationModalService provider Simplify useUniqueFormId

timroes

Tested locally. Seems to work and block correctly as expected. Reviewed code, LGTM. Minor code style comment left.

airbyte-webapp/src/hooks/services/FormChangeTracker/FormChangeTrackerService.tsx

Co-authored-by: Tim Roes <[email protected]>

…11831) * Add navigation blocking when forms are dirty and not saved * Add hooks to block and display prompt on navigation * Add components to indicate which form is blocking * Update connection replication and transformation components to block on edit * Update main page component to show prompt * Add Confimration Modal and Service and update form blocking to use modal * FormNavigationBlocker -> FormChangesTracker useFormNavigationBlocking -> useDiscardFormChangesConfirmation Update exports from default to module * Add confirmation modal text to i18n file * Used formatted title in ConfirmationModal component * Move Form change tracking to a service pattern Fix forms in replications and transformations not resetting the data Add types to import sorting and add Formik helper type for onSubmit functions * Clear all form data after submission in ConnectionForm component Update ConnectionForm submission to handle post submission result from callback * ConnectionForm now clears its own form tracking Add hook to generate unique form id Update form tracker component to use new hook if no id is specified * Adjust margins on ConfirmationModal to more closely match design * Update useUniqueFormId hook id generation to exclude trailing slash * Cleanup code based on pull request review * Remove uneeded dependency spread in useConfirmationModalService hook * Remove data-id from confirmation modal component Update FormChangeTracker to useEffect over useLayoutEffect Memoize useConfirmationModalService hook Remove memoization of ConfirmationModalService provider Simplify useUniqueFormId * Update FormTrackerService check from reduce to some Co-authored-by: Tim Roes <[email protected]> Co-authored-by: Tim Roes <[email protected]>

github-actions bot added area/frontend area/platform issues related to the platform labels Apr 8, 2022

edmundito force-pushed the form-nav-blocker branch from 82a370d to fba94d8 Compare April 11, 2022 13:32

edmundito marked this pull request as ready for review April 13, 2022 20:01

edmundito requested a review from a team as a code owner April 13, 2022 20:01

edmundito force-pushed the form-nav-blocker branch from e59af21 to 86acc12 Compare April 14, 2022 17:46

krishnaglick reviewed Apr 14, 2022

View reviewed changes

edmundito added 11 commits April 18, 2022 09:57

Add Confimration Modal and Service and update form blocking to use modal

78a1829

FormNavigationBlocker -> FormChangesTracker

341f4dd

useFormNavigationBlocking -> useDiscardFormChangesConfirmation Update exports from default to module

Add confirmation modal text to i18n file

362d1aa

Used formatted title in ConfirmationModal component

c625690

Move Form change tracking to a service pattern

cd21133

Fix forms in replications and transformations not resetting the data Add types to import sorting and add Formik helper type for onSubmit functions

Clear all form data after submission in ConnectionForm component

2bd5419

Update ConnectionForm submission to handle post submission result from callback

ConnectionForm now clears its own form tracking

ba5b3b9

Add hook to generate unique form id Update form tracker component to use new hook if no id is specified

Adjust margins on ConfirmationModal to more closely match design

9d4fa10

Update useUniqueFormId hook id generation to exclude trailing slash

e2a42af

Cleanup code based on pull request review

53fdc84

edmundito force-pushed the form-nav-blocker branch from 86acc12 to 53fdc84 Compare April 18, 2022 13:57

Remove uneeded dependency spread in useConfirmationModalService hook

69ba07b

edmundito requested review from krishnaglick and timroes April 18, 2022 17:21

timroes reviewed Apr 19, 2022

View reviewed changes

Remove data-id from confirmation modal component

fae55d7

Update FormChangeTracker to useEffect over useLayoutEffect Memoize useConfirmationModalService hook Remove memoization of ConfirmationModalService provider Simplify useUniqueFormId

edmundito requested a review from timroes April 19, 2022 18:06

timroes approved these changes Apr 20, 2022

View reviewed changes

airbyte-webapp/src/hooks/services/FormChangeTracker/FormChangeTrackerService.tsx Outdated Show resolved Hide resolved

Update FormTrackerService check from reduce to some

3c76502

Co-authored-by: Tim Roes <[email protected]>

edmundito merged commit e1d0525 into master Apr 20, 2022

edmundito deleted the form-nav-blocker branch April 20, 2022 13:36

This was referenced Apr 20, 2022

Bump Airbyte version from 0.36.1-alpha to 0.36.2-alpha #12194

Closed

Bump Airbyte version from 0.36.1-alpha to 0.36.2-alpha #12258

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

edmundito commented Apr 8, 2022 •

edited by timroes

Loading

CLAassistant commented Apr 8, 2022 •

edited

Loading

edmundito commented Apr 14, 2022

krishnaglick Apr 14, 2022

timroes Apr 14, 2022

krishnaglick Apr 14, 2022

krishnaglick Apr 14, 2022

edmundito Apr 18, 2022

krishnaglick Apr 14, 2022

krishnaglick Apr 14, 2022

edmundito Apr 18, 2022

krishnaglick Apr 18, 2022

krishnaglick Apr 14, 2022

krishnaglick Apr 14, 2022

timroes Apr 19, 2022

krishnaglick Apr 14, 2022

krishnaglick Apr 14, 2022

edmundito Apr 18, 2022

timroes Apr 19, 2022

timroes Apr 19, 2022

timroes Apr 19, 2022

edmundito Apr 19, 2022 •

edited

Loading

timroes Apr 19, 2022

edmundito Apr 19, 2022

timroes Apr 19, 2022

timroes Apr 19, 2022

edmundito Apr 19, 2022

timroes left a comment

		@@ -0,0 +1 @@
		export { ConfirmationModal } from "./ConfirmationModal";

		@@ -0,0 +1,3 @@
		import FormChangeTracker from "./FormChangeTracker";

		export default FormChangeTracker;

	if (!!changedFormsById?.[id] !== changed) {
	if (Boolean(changedFormsById?.[id]) !== changed) {

		@@ -0,0 +1,2 @@
		export * from "./FormChangeTrackerService";
		export * from "./hooks";

		@@ -0,0 +1,3 @@
		import { FormikConfig } from "formik";

		export type FormikOnSubmit<T> = FormikConfig<T>["onSubmit"];


		const { trackFormChange } = useFormChangeTrackerService();

		useLayoutEffect(() => {

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

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

Conversation

edmundito commented Apr 8, 2022 • edited by timroes Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Tests

CLAassistant commented Apr 8, 2022 • edited Loading

edmundito commented Apr 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundito Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes left a comment

Choose a reason for hiding this comment

edmundito commented Apr 8, 2022 •

edited by timroes

Loading

CLAassistant commented Apr 8, 2022 •

edited

Loading

edmundito Apr 19, 2022 •

edited

Loading