Feature/better apply and close ux

RELEASE.md

@@ -5,13 +5,14 @@ Please follow the established format:
 - Use present tense (e.g. 'Add new feature')
 - Include the ID number for the related PR (or PRs) in parentheses
 -->
-## Major features and improvements
 
+## Major features and improvements
 
 ## Bug fixes and other changes
 
 - Added warning message when filtered pipeline is empty. (#864)
 - Disabled uvicorn's logger so that log messages are no longer duplicated. (#870)
+- Enhance _Apply and close_ behavior of modals. (#875)
 - Fix namespace collison when two different registered pipelines have a modular pipeline with the same name. (#871)
 
 # Release 4.6.0

src/components/experiment-tracking/details/details.js

@@ -41,9 +41,9 @@ const Details = ({
   return (
     <>
       <RunDetailsModal
-        onClose={setShowRunDetailsModal}
-        runs={runMetadata}
         runMetadataToEdit={runMetadataToEdit}
+        runs={runMetadata}
+        setShowRunDetailsModal={setShowRunDetailsModal}
         theme={theme}
         visible={showRunDetailsModal}
       />

src/components/experiment-tracking/run-details-modal/run-details-modal.js

@@ -1,21 +1,33 @@
 import React, { useEffect, useState } from 'react';
 import { useUpdateRunDetails } from '../../../apollo/mutations';
+
 import Button from '../../ui/button';
 import Modal from '../../ui/modal';
 import Input from '../../ui/input';
 
 import '../../settings-modal/settings-modal.css';
 import './run-details-modal.css';
 
-const RunDetailsModal = ({ onClose, runMetadataToEdit, theme, visible }) => {
+const RunDetailsModal = ({
+  runMetadataToEdit,
+  setShowRunDetailsModal,
+  theme,
+  visible,
+}) => {
   const [valuesToUpdate, setValuesToUpdate] = useState({});
+  const [hasNotInteracted, setHasNotInteracted] = useState(true);
+  const [editsAreSuccessful, setEditsAreSuccessful] = useState(false);
   const { updateRunDetails, error, reset } = useUpdateRunDetails();
 
   const onApplyChanges = () => {
     updateRunDetails({
       runId: runMetadataToEdit.id,
       runInput: { notes: valuesToUpdate.notes, title: valuesToUpdate.title },
     });
+
+    if (!error) {
+      setEditsAreSuccessful(true);
+    }
   };
 
   const onChange = (key, value) => {
@@ -24,8 +36,34 @@ const RunDetailsModal = ({ onClose, runMetadataToEdit, theme, visible }) => {
         [key]: value,
       })
     );
+    setHasNotInteracted(false);
+  };
+
+  const resetState = () => {
+    setHasNotInteracted(true);
+    setEditsAreSuccessful(false);
   };
 
+  useEffect(() => {
+    let modalTimeout, resetTimeout;
+
+    if (editsAreSuccessful) {
+      modalTimeout = setTimeout(() => {
+        setShowRunDetailsModal(false);
+      }, 1500);
+
+      // Delay the reset so the user can't see the button text change.
+      resetTimeout = setTimeout(() => {
+        resetState();
+      }, 2000);
+    }
+
+    return () => {
+      clearTimeout(modalTimeout);
+      clearTimeout(resetTimeout);
+    };
+  }, [editsAreSuccessful, setShowRunDetailsModal]);
+
   useEffect(() => {
     setValuesToUpdate({
       notes: runMetadataToEdit?.notes,
@@ -40,12 +78,13 @@ const RunDetailsModal = ({ onClose, runMetadataToEdit, theme, visible }) => {
      * the next time the modal opens.
      */
     reset();
-  }, [reset, visible]);
+    setHasNotInteracted(true);
+  }, [reset, runMetadataToEdit, visible]);
 
   return (
     <div className="pipeline-settings-modal pipeline-settings-modal--experiment-tracking">
       <Modal
-        onClose={() => onClose(false)}
+        closeModal={() => setShowRunDetailsModal(false)}
         theme={theme}
         title="Edit run details"
         visible={visible}
@@ -57,6 +96,7 @@ const RunDetailsModal = ({ onClose, runMetadataToEdit, theme, visible }) => {
           <Input
             defaultValue={runMetadataToEdit?.title}
             onChange={(value) => onChange('title', value)}
+            resetValueTrigger={visible}
             size="large"
           />
         </div>
@@ -74,11 +114,26 @@ const RunDetailsModal = ({ onClose, runMetadataToEdit, theme, visible }) => {
           />
         </div>
         <div className="run-details-modal-button-wrapper">
-          <Button mode="secondary" onClick={() => onClose(false)} size="small">
+          <Button
+            mode="secondary"
+            onClick={() => setShowRunDetailsModal(false)}
+            size="small"
+          >
             Cancel
           </Button>
-          <Button onClick={onApplyChanges} size="small">
-            Apply changes
+          <Button
+            disabled={hasNotInteracted}
+            onClick={onApplyChanges}
+            mode={editsAreSuccessful ? 'success' : 'primary'}
+            size="small"
+          >
+            {editsAreSuccessful ? (
+              <>
+                Changes applied <span className="success-check-mark">✅</span>
+              </>
+            ) : (
+              'Apply changes and close'
+            )}
           </Button>
         </div>
         {error ? (

src/components/experiment-tracking/run-details-modal/run-details-modal.test.js

@@ -2,6 +2,7 @@ import React from 'react';
 import RunDetailsModal from './index';
 import Adapter from 'enzyme-adapter-react-16';
 import { configure, mount, shallow } from 'enzyme';
+import { render } from '@testing-library/react';
 
 configure({ adapter: new Adapter() });
 
@@ -32,12 +33,20 @@ describe('RunDetailsModal', () => {
     ).toBe(1);
   });
 
-  it('modal closes when X button is clicked', () => {
+  it('renders with a disabled primary button', () => {
+    const { getByText } = render(<RunDetailsModal visible />);
+
+    expect(getByText(/Apply changes and close/i)).toBeDisabled();
+  });
+
+  it('modal closes when cancel button is clicked', () => {
     const setVisible = jest.fn();
-    const wrapper = mount(<RunDetailsModal onClose={() => setVisible(true)} />);
+    const wrapper = mount(
+      <RunDetailsModal setShowRunDetailsModal={() => setVisible(true)} />
+    );
     const onClick = jest.spyOn(React, 'useState');
     const closeButton = wrapper.find(
-      '.pipeline-settings-modal--experiment-tracking .modal__close-button.pipeline-icon-toolbar__button'
+      '.pipeline-settings-modal--experiment-tracking .button__btn.button__btn--secondary'
     );
 
     onClick.mockImplementation((visible) => [visible, setVisible]);
@@ -50,14 +59,4 @@ describe('RunDetailsModal', () => {
       ).length
     ).toBe(0);
   });
-
-  it('calls the updateRunDetails function', () => {
-    const wrapper = mount(
-      <RunDetailsModal runMetadataToEdit={{ id: 'test' }} visible={true} />
-    );
-
-    wrapper.find('.button__btn--primary').simulate('click');
-
-    expect(mockUpdateRunDetails).toHaveBeenCalled();
-  });
 });

src/components/export-modal/export-modal.js

@@ -15,8 +15,8 @@ const ExportModal = ({ graphSize, theme, onToggle, visible }) => {
   }
   return (
     <Modal
+      closeModal={() => onToggle(false)}
       title="Export pipeline visualisation"
-      onClose={() => onToggle(false)}
       visible={visible.exportModal}
     >
       <div className="pipeline-export-modal">

src/components/export-modal/export-modal.test.js

@@ -17,9 +17,6 @@ describe('ExportModal', () => {
     };
     const wrapper = mount();
     expect(wrapper.find('.modal__content--visible').length).toBe(1);
-    const closeButton = wrapper.find('.modal__close-button');
-    closeButton.find('button').simulate('click');
-    expect(wrapper.find('.modal__content--visible').length).toBe(0);
   });
 
   it('maps state to props', () => {

src/components/settings-modal/settings-modal.js

@@ -1,6 +1,5 @@
-import React from 'react';
+import React, { useEffect, useState } from 'react';
 import { connect } from 'react-redux';
-import Modal from '../ui/modal';
 import {
   changeFlag,
   toggleSettingsModal,
@@ -9,6 +8,10 @@ import {
 import { getFlagsState } from '../../utils/flags';
 import SettingsModalRow from './settings-modal-row';
 import { settings as settingsConfig } from '../../config';
+
+import Modal from '../ui/modal';
+import Button from '../ui/button';
+
 import './settings-modal.css';
 
 /**
@@ -18,21 +21,49 @@ import './settings-modal.css';
 const SettingsModal = ({
   flags,
   isOutdated,
-  onClose,
+  latestVersion,
+  showSettingsModal,
   onToggleFlag,
   onTogglePrettyName,
-  latestVersion,
   prettyName,
   visible,
-  theme,
 }) => {
+  const [hasNotInteracted, setHasNotInteracted] = useState(true);
+  const [hasClickedApplyAndClose, setHasClickApplyAndClose] = useState(false);
   const flagData = getFlagsState();
 
+  useEffect(() => {
+    let modalTimeout, resetTimeout;
+
+    if (hasClickedApplyAndClose) {
+      modalTimeout = setTimeout(() => {
+        showSettingsModal(false);
+      }, 1500);
+
+      // Delay the reset so the user can't see the button text change.
+      resetTimeout = setTimeout(() => {
+        setHasNotInteracted(true);
+        setHasClickApplyAndClose(false);
+        window.location.reload();
+      }, 2000);
+    }
+
+    return () => {
+      clearTimeout(modalTimeout);
+      clearTimeout(resetTimeout);
+    };
+  }, [hasClickedApplyAndClose, showSettingsModal]);
+
+  const resetStateCloseModal = () => {
+    showSettingsModal(false);
+    setHasNotInteracted(true);
+  };
+
   return (
     <div className="pipeline-settings-modal">
       <Modal
+        closeModal={() => resetStateCloseModal()}
         title="Settings"
-        onClose={() => onClose(false)}
         visible={visible.settingsModal}
       >
         <div className="pipeline-settings-modal__content">
@@ -49,19 +80,23 @@ const SettingsModal = ({
             name={settingsConfig['prettyName'].name}
             toggleValue={prettyName}
             description={settingsConfig['prettyName'].description}
-            onToggleChange={(event) => onTogglePrettyName(event.target.checked)}
+            onToggleChange={(event) => {
+              onTogglePrettyName(event.target.checked);
+              setHasNotInteracted(false);
+            }}
           />
           <div className="pipeline-settings-modal__subtitle">Experiments</div>
-          {flagData.map(({ name, value, description }, index) => (
+          {flagData.map(({ name, value, description }) => (
             <SettingsModalRow
-              key={value}
+              description={description}
               id={value}
+              key={value}
               name={name}
+              onToggleChange={(event) => {
+                onToggleFlag(value, event.target.checked);
+                setHasNotInteracted(false);
+              }}
               toggleValue={flags[value]}
-              description={description}
-              onToggleChange={(event) =>
-                onToggleFlag(value, event.target.checked)
-              }
             />
           ))}
           {isOutdated ? (
@@ -83,21 +118,48 @@ const SettingsModal = ({
               </span>
             </div>
           )}
+          <div className="run-details-modal-button-wrapper">
+            <Button
+              mode="secondary"
+              onClick={() => {
+                showSettingsModal(false);
+                setHasNotInteracted(true);
+              }}
+              size="small"
+            >
+              Cancel
+            </Button>
+            <Button
+              disabled={hasNotInteracted}
+              onClick={() => {
+                setHasClickApplyAndClose(true);
+              }}
+              mode={hasClickedApplyAndClose ? 'success' : 'primary'}
+              size="small"
+            >
+              {hasClickedApplyAndClose ? (
+                <>
+                  Changes applied <span className="success-check-mark">✅</span>
+                </>
+              ) : (
+                'Apply changes and close'
+              )}
+            </Button>
+          </div>
         </div>
       </Modal>
     </div>
   );
 };
 
 export const mapStateToProps = (state) => ({
-  visible: state.visible,
-  theme: state.theme,
-  prettyName: state.prettyName,
   flags: state.flags,
+  prettyName: state.prettyName,
+  visible: state.visible,
 });
 
 export const mapDispatchToProps = (dispatch) => ({
-  onClose: (value) => {
+  showSettingsModal: (value) => {
     dispatch(toggleSettingsModal(value));
   },
   onToggleFlag: (name, value) => {

src/components/settings-modal/settings-modal.scss

@@ -37,9 +37,8 @@
 }
 
 .pipeline-settings-modal__content {
-  width: 100%;
-  margin-bottom: 4em;
   font-size: 1.25em;
+  width: 100%;
 
   &--short {
     margin-bottom: 2.4em;
@@ -90,4 +89,5 @@
 
 .pipeline-settings-modal__already-latest {
   color: var(--color-modal-header-text);
+  margin-bottom: 2.4em;
 }