Skip to content

ERA-11154: Do not add a collection item when pressing "cancel" upon first creation #1272

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

Closed
wants to merge 2 commits into from
Closed

ERA-11154: Do not add a collection item when pressing "cancel" upon first creation #1272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
957d0e2
WIP: validating editing items on collections
Mar 31, 2025
7da2cca
Adding logic to remove empty collection items when canceling an item …
Apr 1, 2025
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: 2 additions & 0 deletions src/ReportManager/DetailsSection/SchemaForm/fields/Collection/SortableList/Item/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const Item = ({
isFormPreviewOpen,
onChange = null,
onDelete = null,
onCancel = null,
renderField = null,
setIsFormModalOpen = null,
setIsFormPreviewOpen = null,
Expand Down Expand Up @@ -77,6 +78,7 @@ const Item = ({
const onFormModalCancel = () => {
onChange(formDataBeforeEditingRef.current, errorsBeforeEditingRef.current);
setIsFormModalOpen(false);
onCancel?.();
};

const onFieldChange = (fieldId, value, error) => {
Expand Down
2 changes: 2 additions & 0 deletions src/ReportManager/DetailsSection/SchemaForm/fields/Collection/SortableList/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ const SortableList = ({
focusLocationMarker,
items,
onItemChange,
onItemCancel,
onItemDelete,
onItemMove,
renderField,
Expand Down Expand Up @@ -155,6 +156,7 @@ const SortableList = ({
isFormPreviewOpen={item.isFormPreviewOpen}
key={item.id}
onChange={onItemChange(index)}
onCancel={onItemCancel(index)}
onDelete={onItemDelete(index)}
setIsFormModalOpen={setIsItemFormModalOpen(index)}
setIsFormPreviewOpen={setIsItemFormPreviewOpen(index)}
Expand Down
11 changes: 10 additions & 1 deletion src/ReportManager/DetailsSection/SchemaForm/fields/Collection/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ const Collection = ({
if (erroneousItemIndex > itemIndex) {
updatedError[parseInt(erroneousItemIndex) - 1] = updatedError[erroneousItemIndex];
delete updatedError[erroneousItemIndex];
};
}
});
}

Expand Down Expand Up @@ -133,6 +133,14 @@ const Collection = ({
setItems([...items, { id: lastAddedItemIdRef.current, isFormModalOpen: true, isFormPreviewOpen: false }]);
};

const onItemCancel = (itemIndex) => () => {
if (Object.keys(value[itemIndex]).length === 0){ // applying the removal logic only for new items
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this logic matches the requirement 🤔

What I read here is: when the user clicks cancel on a collection item modal, if the inner fields of the item don't have values yet, delete it. This means that items that were not recently added could be deleted with Cancel if they are empty, but that's not the idea, we have a trash button for that. Cancel is designed just to "undo" the changes done while having the modal open.

The requirement from design is: when the user adds a new item and immediately clicks cancel, delete the item (only on that first time the modal opened automatically after adding the item). In that first render of the modal we want the cancel button to work as a delete because from the user perspective "Cancel" would mean to cancel the "addition" of the item, not to cancel the changes.

I think there's a very simple approach using a ref to track if the item was just added without having to pass too many props around. What I would have expected is to add some logic in the Item component like:

const Item = ({...}) => {
  const errorsBeforeEditingRef = useRef(null);
  const formDataBeforeEditingRef = useRef(null);
  const shouldDeleteOnCancelRef = useRef(true);  // <--- New Ref
  ...
  const onFormModalCancel = () => {
    // New if: if shouldDeleteOnCancelRef.current is true, the item was just added, so if the user clicks cancel we delete it.
    if (shouldDeleteOnCancelRef.current) {
      onDeleteItem();
    } else {
      onChange(formDataBeforeEditingRef.current, errorsBeforeEditingRef.current);
      setIsFormModalOpen(false);
    }
  };
  ...
  // New callback to attach to the <FormModal onDone={onFormModalDone} />
  const onFormModalDone = () => {
    setIsFormModalOpen(false);

    // If the user clicks Done, we now set the ref as false so next time the modal opens we don't delete it on cancel.
    shouldDeleteOnCancelRef.current = false;
  };
  ...
};

I'm not sure if this code runs perfectly, probably not, but this is the overall idea I have of a simple implementation of the requirements. Also I remember we said that the trash icon at the bottom left of the modal shouldn't show up in that first modal opening, since the cancel button already works as a delete:
image

With the approach I suggest, now you have a flag shouldDeleteOnCancelRef.current that you can use to conditionally render the trash button 😄

lastAddedItemIdRef.current -= 1;
setItems(items.filter((_, index) => itemIndex !== index));
onFieldChange(id, value.filter((_, index) => itemIndex !== index));
}
};

// If a location field from an item requests to focus its location marker, prefix the marker id with the collection
// id and the item index.
const focusLocationMarkerFromItem = (itemIndex) => (markerId) =>
Expand Down Expand Up @@ -184,6 +192,7 @@ const Collection = ({
onItemChange={onItemChange}
onItemDelete={onItemDelete}
onItemMove={onItemMove}
onItemCancel={onItemCancel}
setIsItemFormModalOpen={setIsItemFormModalOpen}
setIsItemFormPreviewOpen={setIsItemFormPreviewOpen}
renderField={renderField}
Expand Down