fix: fix field select options positions after option removal (twentyh…

…q#5350) - Adds an util `toSpliced`. We cannot used the native Javascript `Array.prototype.toSpliced` method as Chromatic servers don't support it. - Makes sure Select field options have sequential positions after removing an option (form validation schema checks that positions are sequential and considers options invalid otherwise).
synapsenet-arena · May 10, 2024 · 0fb416d · 0fb416d
1 parent 72521d5
commit 0fb416d
Show file tree

Hide file tree

Showing 5 changed files with 99 additions and 16 deletions.
diff --git a/...twenty-front/src/modules/settings/data-model/components/SettingsObjectFieldSelectForm.tsx b/...twenty-front/src/modules/settings/data-model/components/SettingsObjectFieldSelectForm.tsx
@@ -19,6 +19,7 @@ import { DraggableItem } from '@/ui/layout/draggable-list/components/DraggableIt
 import { DraggableList } from '@/ui/layout/draggable-list/components/DraggableList';
 import { FieldMetadataType } from '~/generated-metadata/graphql';
 import { moveArrayItem } from '~/utils/array/moveArrayItem';
+import { toSpliced } from '~/utils/array/toSpliced';
 import { applySimpleQuotesToString } from '~/utils/string/applySimpleQuotesToString';
 import { simpleQuotesStringSchema } from '~/utils/validation-schemas/simpleQuotesStringSchema';
 
@@ -198,8 +199,12 @@ export const SettingsDataModelFieldSelectForm = ({
                             key={option.id}
                             option={option}
                             onChange={(nextOption) => {
-                              const nextOptions = [...options];
-                              nextOptions.splice(index, 1, nextOption);
+                              const nextOptions = toSpliced(
+                                options,
+                                index,
+                                1,
+                                nextOption,
+                              );
                               onChange(nextOptions);
 
                               // Update option value in defaultValue if value has changed
@@ -211,15 +216,17 @@ export const SettingsDataModelFieldSelectForm = ({
                                 handleSetOptionAsDefault(nextOption.value);
                               }
                             }}
-                            onRemove={
-                              options.length > 1
-                                ? () => {
-                                    const nextOptions = [...options];
-                                    nextOptions.splice(index, 1);
-                                    onChange(nextOptions);
-                                  }
-                                : undefined
-                            }
+                            onRemove={() => {
+                              const nextOptions = toSpliced(
+                                options,
+                                index,
+                                1,
+                              ).map((option, nextOptionIndex) => ({
+                                ...option,
+                                position: nextOptionIndex,
+                              }));
+                              onChange(nextOptions);
+                            }}
                             isDefault={isOptionDefaultValue(option.value)}
                             onSetAsDefault={() =>
                               handleSetOptionAsDefault(option.value)

diff --git a/...ages/twenty-front/src/modules/ui/utilities/pointer-event/hooks/useClickOutsideListener.ts b/...ages/twenty-front/src/modules/ui/utilities/pointer-event/hooks/useClickOutsideListener.ts
@@ -8,6 +8,7 @@ import {
 } from '@/ui/utilities/pointer-event/hooks/useListenClickOutsideV2';
 import { ClickOutsideListenerCallback } from '@/ui/utilities/pointer-event/types/ClickOutsideListenerCallback';
 import { getScopeIdFromComponentId } from '@/ui/utilities/recoil-scope/utils/getScopeIdFromComponentId';
+import { toSpliced } from '~/utils/array/toSpliced';
 import { isDefined } from '~/utils/isDefined';
 
 export const useClickOutsideListener = (componentId: string) => {
@@ -117,8 +118,11 @@ export const useClickOutsideListener = (componentId: string) => {
         const callbackToUnsubscribeIsFound = indexOfCallbackToUnsubscribe > -1;
 
         if (callbackToUnsubscribeIsFound) {
-          const newCallbacksWithoutCallbackToUnsubscribe =
-            existingCallbacks.toSpliced(indexOfCallbackToUnsubscribe, 1);
+          const newCallbacksWithoutCallbackToUnsubscribe = toSpliced(
+            existingCallbacks,
+            indexOfCallbackToUnsubscribe,
+            1,
+          );
 
           set(
             getClickOutsideListenerCallbacksState,

diff --git a/packages/twenty-front/src/utils/array/__tests__/toSpliced.test.ts b/packages/twenty-front/src/utils/array/__tests__/toSpliced.test.ts
@@ -0,0 +1,37 @@
+import { toSpliced } from '../toSpliced';
+
+describe('toSpliced', () => {
+  it('removes elements from the array starting at the given index', () => {
+    // Given
+    const array = ['a', 'b', 'c', 'd', 'e'];
+
+    // When
+    const result = toSpliced(array, 2, 2);
+
+    // Then
+    expect(result).toEqual(['a', 'b', 'e']);
+  });
+
+  it('replaces elements in the array starting at the given index', () => {
+    // Given
+    const array = ['a', 'b', 'c', 'd', 'e'];
+
+    // When
+    const result = toSpliced(array, 1, 2, 'x', 'y');
+
+    // Then
+    expect(result).toEqual(['a', 'x', 'y', 'd', 'e']);
+  });
+
+  it('returns a new array without modifying the original array', () => {
+    // Given
+    const array = ['a', 'b', 'c'];
+
+    // When
+    const result = toSpliced(array, 0, 1);
+
+    // Then
+    expect(result).toEqual(['b', 'c']);
+    expect(array).toEqual(['a', 'b', 'c']);
+  });
+});
diff --git a/packages/twenty-front/src/utils/array/moveArrayItem.ts b/packages/twenty-front/src/utils/array/moveArrayItem.ts
@@ -1,3 +1,5 @@
+import { toSpliced } from '~/utils/array/toSpliced';
+
 /**
  * Moves an item in an array from one index to another.
  *
@@ -20,9 +22,14 @@ export const moveArrayItem = <ArrayItem>(
     return array;
   }
 
-  const arrayWithMovedItem = [...array];
-  const [itemToMove] = arrayWithMovedItem.splice(fromIndex, 1);
-  arrayWithMovedItem.splice(toIndex, 0, itemToMove);
+  const itemToMove = array[fromIndex];
+  const arrayWithoutItem = toSpliced(array, fromIndex, 1);
+  const arrayWithMovedItem = toSpliced(
+    arrayWithoutItem,
+    toIndex,
+    0,
+    itemToMove,
+  );
 
   return arrayWithMovedItem;
 };
diff --git a/packages/twenty-front/src/utils/array/toSpliced.ts b/packages/twenty-front/src/utils/array/toSpliced.ts
@@ -0,0 +1,28 @@
+type ToSplicedFn = {
+  <T>(array: T[], start: number, deleteCount?: number): T[];
+  <T>(array: T[], start: number, deleteCount: number, ...items: T[]): T[];
+};
+
+/**
+ * Returns a new array with some elements removed and/or replaced at a given index.
+ * This does the same as `Array.prototype.toSpliced`.
+ * We cannot use the native `Array.prototype.toSpliced` method as of now because Chromatic's runners do not support it.
+ *
+ * @param array - The array to remove and/or replace elements from.
+ * @param start - The index at which to start changing the array.
+ * @param deleteCount - The number of elements in the array to remove from `start`.
+ * @param items - The elements to add to the array at `start`.
+ *
+ * @returns A new array with elements removed and/or replaced at a given index.
+ *
+ * @example
+ * toSpliced(['a', 'b', 'c'], 0, 1)
+ * => ['b', 'c']
+ * toSpliced(['a', 'b', 'c'], 0, 1, 'd')
+ * => ['d', 'b', 'c']
+ */
+export const toSpliced: ToSplicedFn = (array, ...args) => {
+  const arrayCopy = [...array];
+  arrayCopy.splice(...(args as [number, number, ...any[]]));
+  return arrayCopy;
+};