refactor: refactor edit base item form (#1582)

* refactor: refactor edit base item form

* refactor: fix tests

cypress/e2e/item/settings/itemSettings.cy.ts

@@ -1,5 +1,4 @@
 import {
-  DescriptionPlacement,
   ItemType,
   MaxWidth,
   MimeTypes,
@@ -25,14 +24,12 @@ import {
   ITEM_MAIN_CLASS,
   ITEM_PANEL_NAME_ID,
   ITEM_PANEL_TABLE_ID,
-  ITEM_SETTING_DESCRIPTION_PLACEMENT_SELECT_ID,
   LANGUAGE_SELECTOR_ID,
   SETTINGS_CHATBOX_TOGGLE_ID,
   SETTINGS_LINK_SHOW_BUTTON_ID,
   SETTINGS_LINK_SHOW_IFRAME_ID,
   SETTINGS_PINNED_TOGGLE_ID,
   SETTINGS_SAVE_ACTIONS_TOGGLE_ID,
-  buildDescriptionPlacementId,
   buildItemsGridMoreButtonSelector,
   buildSettingsButtonId,
 } from '../../../../src/config/selectors';
@@ -446,42 +443,6 @@ describe('Item Settings', () => {
         );
       });
     });
-
-    describe('DescriptionPlacement Settings', () => {
-      it('folder should not have description placement', () => {
-        const FOLDER = PackedFolderItemFactory();
-        cy.setUpApi({ items: [FOLDER] });
-        const { id, type } = FOLDER;
-
-        cy.visit(buildItemSettingsPath(id));
-
-        cy.get(`#${ITEM_PANEL_TABLE_ID}`).contains(type);
-
-        cy.get(`#${ITEM_SETTING_DESCRIPTION_PLACEMENT_SELECT_ID}`).should(
-          'not.exist',
-        );
-      });
-
-      it('update placement to above for file', () => {
-        const LINK = PackedLinkItemFactory();
-        cy.setUpApi({ items: [LINK] });
-        const { id } = LINK;
-
-        cy.visit(buildItemSettingsPath(id));
-
-        cy.get(`#${ITEM_SETTING_DESCRIPTION_PLACEMENT_SELECT_ID}`).click();
-        cy.get(
-          `#${buildDescriptionPlacementId(DescriptionPlacement.ABOVE)}`,
-        ).click();
-
-        cy.wait(`@editItem`).then(({ request: { url, body } }) => {
-          expect(url).to.contain(id);
-          expect(body?.settings).to.contain({
-            descriptionPlacement: DescriptionPlacement.ABOVE,
-          });
-        });
-      });
-    });
   });
 
   describe('in item menu', () => {

src/components/item/edit/EditModal.tsx

@@ -1,30 +1,13 @@
 import { ComponentType as CT, useEffect, useState } from 'react';
 import { toast } from 'react-toastify';
 
-import {
-  Button,
-  Dialog,
-  DialogActions,
-  DialogContent,
-  DialogTitle,
-} from '@mui/material';
+import { Dialog, DialogTitle } from '@mui/material';
 
-import { routines } from '@graasp/query-client';
 import { DiscriminatedItem, ItemType } from '@graasp/sdk';
-import { COMMON, FAILURE_MESSAGES } from '@graasp/translations';
+import { FAILURE_MESSAGES } from '@graasp/translations';
 
-import isEqual from 'lodash.isequal';
-
-import CancelButton from '@/components/common/CancelButton';
-import { useBuilderTranslation, useCommonTranslation } from '@/config/i18n';
-import notifier from '@/config/notifier';
-import { mutations } from '@/config/queryClient';
-import {
-  EDIT_ITEM_MODAL_CANCEL_BUTTON_ID,
-  EDIT_MODAL_ID,
-  ITEM_FORM_CONFIRM_BUTTON_ID,
-} from '@/config/selectors';
-import { isItemValid } from '@/utils/item';
+import { useBuilderTranslation, useMessagesTranslation } from '@/config/i18n';
+import { EDIT_MODAL_ID } from '@/config/selectors';
 
 import { BUILDER } from '../../../langs/constants';
 import BaseItemForm from '../form/BaseItemForm';
@@ -33,8 +16,6 @@ import FileForm from '../form/file/FileForm';
 import { FolderEditForm } from '../form/folder/FolderEditForm';
 import EditShortcutForm from '../shortcut/EditShortcutForm';
 
-const { editItemRoutine } = routines;
-
 export interface EditModalContentPropType {
   item?: DiscriminatedItem;
   setChanges: (payload: Partial<DiscriminatedItem>) => void;
@@ -49,9 +30,7 @@ type Props = {
 
 const EditModal = ({ item, onClose, open }: Props): JSX.Element => {
   const { t: translateBuilder } = useBuilderTranslation();
-  const { t: translateCommon } = useCommonTranslation();
-
-  const { mutate: editItem } = mutations.useEditItem();
+  const { t: translateMessage } = useMessagesTranslation();
 
   // updated properties are separated from the original item
   // so only necessary properties are sent when editing
@@ -63,59 +42,6 @@ const EditModal = ({ item, onClose, open }: Props): JSX.Element => {
     }
   }, [item, updatedItem.id]);
 
-  const setChanges = (payload: Partial<DiscriminatedItem>) => {
-    setUpdatedItem({ ...updatedItem, ...payload } as DiscriminatedItem);
-  };
-
-  // files, folders, documents are handled beforehand
-  const renderDialogContent = (): JSX.Element => {
-    switch (item.type) {
-      case ItemType.LINK:
-      case ItemType.APP:
-      case ItemType.ETHERPAD:
-      case ItemType.H5P:
-      default:
-        return <BaseItemForm setChanges={setChanges} item={item} />;
-    }
-  };
-
-  const submit = () => {
-    if (
-      !isItemValid({
-        ...item,
-        ...updatedItem,
-      } as DiscriminatedItem)
-    ) {
-      toast.error<string>(translateBuilder(BUILDER.EDIT_ITEM_ERROR_MESSAGE));
-      return;
-    }
-
-    // add id to changed properties
-    if (!item?.id) {
-      notifier({
-        type: editItemRoutine.FAILURE,
-        payload: { error: new Error(FAILURE_MESSAGES.UNEXPECTED_ERROR) },
-      });
-    } else {
-      editItem({
-        id: updatedItem.id,
-        name: updatedItem.name,
-        description: updatedItem.description,
-        // only post extra if it has been changed
-        // todo: fix type
-        extra: !isEqual(item.extra, updatedItem.extra)
-          ? (updatedItem.extra as any)
-          : undefined,
-        // only patch settings it it has been changed
-        ...(!isEqual(item.settings, updatedItem.settings)
-          ? { settings: updatedItem.settings }
-          : {}),
-      });
-    }
-
-    onClose();
-  };
-
   // temporary solution for displaying separate dialog content
   const renderContent = () => {
     if (item.type === ItemType.FOLDER) {
@@ -130,41 +56,18 @@ const EditModal = ({ item, onClose, open }: Props): JSX.Element => {
     if (item.type === ItemType.DOCUMENT) {
       return <DocumentEditForm onClose={onClose} item={item} />;
     }
+    if (
+      item.type === ItemType.LINK ||
+      item.type === ItemType.ETHERPAD ||
+      item.type === ItemType.APP ||
+      item.type === ItemType.H5P
+    ) {
+      return <BaseItemForm onClose={onClose} item={item} />;
+    }
+
+    toast.error(translateMessage(FAILURE_MESSAGES.UNEXPECTED_ERROR));
 
-    return (
-      <>
-        <DialogContent
-          sx={{
-            display: 'flex',
-            flexDirection: 'column',
-          }}
-        >
-          {renderDialogContent()}
-        </DialogContent>
-        <DialogActions>
-          <CancelButton
-            id={EDIT_ITEM_MODAL_CANCEL_BUTTON_ID}
-            onClick={onClose}
-          />
-          <Button
-            // should not allow users to save if the item is not valid
-            disabled={
-              // // maybe we do not need the state variable and can just check the item
-              // isConfirmButtonDisabled ||
-              // isItem Valid checks a full item, so we add the updated properties to the item to check
-              !isItemValid({
-                ...item,
-                ...updatedItem,
-              })
-            }
-            onClick={submit}
-            id={ITEM_FORM_CONFIRM_BUTTON_ID}
-          >
-            {translateCommon(COMMON.SAVE_BUTTON)}
-          </Button>
-        </DialogActions>
-      </>
-    );
+    return null;
   };
 
   return (

src/components/item/form/BaseItemForm.tsx

@@ -1,11 +1,19 @@
-import { useEffect } from 'react';
 import { FormProvider, useForm } from 'react-hook-form';
 
-import { Box } from '@mui/material';
+import { LoadingButton } from '@mui/lab';
+import { Box, DialogActions, DialogContent } from '@mui/material';
 
 import { DescriptionPlacementType, DiscriminatedItem } from '@graasp/sdk';
+import { COMMON } from '@graasp/translations';
+
+import CancelButton from '@/components/common/CancelButton';
+import { useCommonTranslation } from '@/config/i18n';
+import { mutations } from '@/config/queryClient';
+import {
+  EDIT_ITEM_MODAL_CANCEL_BUTTON_ID,
+  ITEM_FORM_CONFIRM_BUTTON_ID,
+} from '@/config/selectors';
 
-import { FOLDER_FORM_DESCRIPTION_ID } from '../../../config/selectors';
 import { ItemNameField } from './ItemNameField';
 import { DescriptionAndPlacementForm } from './description/DescriptionAndPlacementForm';
 
@@ -17,47 +25,59 @@ type Inputs = {
 
 const BaseItemForm = ({
   item,
-  setChanges,
+  onClose,
 }: {
   item: DiscriminatedItem;
-  setChanges: (payload: Partial<DiscriminatedItem>) => void;
+  onClose: () => void;
 }): JSX.Element => {
+  const { t: translateCommon } = useCommonTranslation();
   const methods = useForm<Inputs>({ defaultValues: { name: item.name } });
-  const { watch, setValue } = methods;
-  const name = watch('name');
-  const descriptionPlacement = watch('descriptionPlacement');
-  const description = watch('description');
-
-  // synchronize upper state after async local state change
-  useEffect(() => {
-    setChanges({
-      name,
-      description,
-      settings: { descriptionPlacement },
-    });
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [description, descriptionPlacement, name]);
+  const { mutateAsync: editItem, isPending } = mutations.useEditItem();
+  const {
+    setValue,
+    handleSubmit,
+    formState: { isValid },
+  } = methods;
 
+  async function onSubmit(data: Inputs) {
+    try {
+      await editItem({
+        id: item.id,
+        name: data.name,
+        description: data.description,
+        settings: { descriptionPlacement: data.descriptionPlacement },
+      });
+      onClose();
+    } catch (e) {
+      console.error(e);
+    }
+  }
   return (
-    <Box overflow="auto">
+    <Box component="form" onSubmit={handleSubmit(onSubmit)}>
       <FormProvider {...methods}>
-        <ItemNameField required />
-
-        <Box sx={{ mt: 2 }}>
+        <DialogContent>
+          <ItemNameField required />
           <DescriptionAndPlacementForm
-            id={FOLDER_FORM_DESCRIPTION_ID}
-            description={description ?? item?.description}
-            descriptionPlacement={
-              descriptionPlacement ?? item?.settings?.descriptionPlacement
-            }
-            onPlacementChange={(newValue) =>
-              setValue('descriptionPlacement', newValue)
-            }
             onDescriptionChange={(newValue) => {
               setValue('description', newValue);
             }}
           />
-        </Box>
+        </DialogContent>
+        <DialogActions>
+          <CancelButton
+            id={EDIT_ITEM_MODAL_CANCEL_BUTTON_ID}
+            onClick={onClose}
+          />
+          <LoadingButton
+            loading={isPending}
+            id={ITEM_FORM_CONFIRM_BUTTON_ID}
+            type="submit"
+            disabled={!isValid}
+            variant="contained"
+          >
+            {translateCommon(COMMON.SAVE_BUTTON)}
+          </LoadingButton>
+        </DialogActions>
       </FormProvider>
     </Box>
   );

src/components/item/form/description/DescriptionAndPlacementForm.tsx

@@ -1,23 +1,17 @@
 import { Stack } from '@mui/material';
 
-import { DescriptionPlacementType } from '@graasp/sdk';
-
 import { DescriptionForm, type DescriptionFormProps } from './DescriptionForm';
 import DescriptionPlacementForm from './DescriptionPlacementForm';
 
 type DescriptionAndPlacementFormProps = {
   id?: string;
   description?: string;
-  descriptionPlacement?: DescriptionPlacementType;
-  onPlacementChange: (v: DescriptionPlacementType) => void;
   onDescriptionChange: DescriptionFormProps['onChange'];
 };
 
 export const DescriptionAndPlacementForm = ({
   id,
   description = '',
-  descriptionPlacement,
-  onPlacementChange,
   onDescriptionChange,
 }: DescriptionAndPlacementFormProps): JSX.Element => (
   <Stack spacing={2}>
@@ -26,9 +20,6 @@ export const DescriptionAndPlacementForm = ({
       value={description}
       onChange={onDescriptionChange}
     />
-    <DescriptionPlacementForm
-      descriptionPlacement={descriptionPlacement}
-      onPlacementChange={onPlacementChange}
-    />
+    <DescriptionPlacementForm />
   </Stack>
 );