From ab8ad466854c29d6e24a7a51d5792aab40a30bac Mon Sep 17 00:00:00 2001 From: Charles Bochet Date: Fri, 6 Dec 2024 19:19:27 +0100 Subject: [PATCH] Fix bug reoder on table in view groups mode (#8894) Fixes #8403 Added comments on the PR to explain changes --- .../RecordTableNoRecordGroupRows.tsx | 3 +- .../components/RecordTableRecordGroupRows.tsx | 5 +- .../hooks/useComputeNewRowPosition.ts | 90 ------------------- ...RecordTableBodyDragDropContextProvider.tsx | 90 ++++++++++++++----- ...BodyRecordGroupDragDropContextProvider.tsx | 83 ++++++++++++----- .../components/RecordTablePendingRow.tsx | 3 +- .../components/RecordTableRow.tsx | 9 +- .../components/RecordTableRowWrapper.tsx | 14 +-- .../hooks/useRecoilCallbackState.ts | 27 ------ 9 files changed, 150 insertions(+), 174 deletions(-) delete mode 100644 packages/twenty-front/src/modules/object-record/record-table/hooks/useComputeNewRowPosition.ts delete mode 100644 packages/twenty-front/src/modules/ui/utilities/state/component-state/hooks/useRecoilCallbackState.ts diff --git a/packages/twenty-front/src/modules/object-record/record-table/components/RecordTableNoRecordGroupRows.tsx b/packages/twenty-front/src/modules/object-record/record-table/components/RecordTableNoRecordGroupRows.tsx index 35ef9bf2b1..beed4ae065 100644 --- a/packages/twenty-front/src/modules/object-record/record-table/components/RecordTableNoRecordGroupRows.tsx +++ b/packages/twenty-front/src/modules/object-record/record-table/components/RecordTableNoRecordGroupRows.tsx @@ -15,7 +15,8 @@ export const RecordTableNoRecordGroupRows = () => { ); })} diff --git a/packages/twenty-front/src/modules/object-record/record-table/components/RecordTableRecordGroupRows.tsx b/packages/twenty-front/src/modules/object-record/record-table/components/RecordTableRecordGroupRows.tsx index a5487dcfea..01688556de 100644 --- a/packages/twenty-front/src/modules/object-record/record-table/components/RecordTableRecordGroupRows.tsx +++ b/packages/twenty-front/src/modules/object-record/record-table/components/RecordTableRecordGroupRows.tsx @@ -32,7 +32,7 @@ export const RecordTableRecordGroupRows = () => { return ( isRecordGroupTableSectionToggled && - recordIdsByGroup.map((recordId) => { + recordIdsByGroup.map((recordId, rowIndexInGroup) => { const rowIndex = rowIndexMap.get(recordId); if (!isDefined(rowIndex)) { @@ -43,7 +43,8 @@ export const RecordTableRecordGroupRows = () => { ); diff --git a/packages/twenty-front/src/modules/object-record/record-table/hooks/useComputeNewRowPosition.ts b/packages/twenty-front/src/modules/object-record/record-table/hooks/useComputeNewRowPosition.ts deleted file mode 100644 index 0ef47ec49f..0000000000 --- a/packages/twenty-front/src/modules/object-record/record-table/hooks/useComputeNewRowPosition.ts +++ /dev/null @@ -1,90 +0,0 @@ -import { DropResult } from '@hello-pangea/dnd'; -import { useRecoilCallback } from 'recoil'; - -import { recordStoreFamilyState } from '@/object-record/record-store/states/recordStoreFamilyState'; -import { isDefined } from '~/utils/isDefined'; - -export const useComputeNewRowPosition = () => { - return useRecoilCallback( - ({ snapshot }) => - (result: DropResult, tableRowIds: string[]) => { - if (!isDefined(result.destination)) { - return; - } - - const draggedRecordId = result.draggableId; - const destinationIndex = result.destination.index; - const sourceIndex = result.source.index; - - const recordBeforeId = tableRowIds[destinationIndex - 1]; - const recordDestinationId = tableRowIds[destinationIndex]; - const recordAfterDestinationId = tableRowIds[destinationIndex + 1]; - - const recordBefore = recordBeforeId - ? snapshot - .getLoadable(recordStoreFamilyState(recordBeforeId)) - .getValue() - : null; - const recordDestination = recordDestinationId - ? snapshot - .getLoadable(recordStoreFamilyState(recordDestinationId)) - .getValue() - : null; - const recordAfterDestination = recordAfterDestinationId - ? snapshot - .getLoadable(recordStoreFamilyState(recordAfterDestinationId)) - .getValue() - : null; - - const computeNewPosition = (destIndex: number, sourceIndex: number) => { - const moveToFirstPosition = destIndex === 0; - const moveToLastPosition = destIndex === tableRowIds.length - 1; - const moveAfterSource = destIndex > sourceIndex; - - const firstRecord = tableRowIds[0] - ? snapshot - .getLoadable(recordStoreFamilyState(tableRowIds[0])) - .getValue() - : null; - - const lastRecord = tableRowIds[tableRowIds.length - 1] - ? snapshot - .getLoadable( - recordStoreFamilyState(tableRowIds[tableRowIds.length - 1]), - ) - .getValue() - : null; - - const firstRecordPosition = firstRecord?.position ?? 0; - - if (moveToFirstPosition) { - if (firstRecordPosition <= 0) { - return firstRecordPosition - 1; - } else { - return firstRecordPosition / 2; - } - } else if (moveToLastPosition) { - return lastRecord?.position + 1; - } else if (moveAfterSource) { - const recordAfterDestinationPosition = - recordAfterDestination?.position ?? 0; - const recordDestinationPosition = recordDestination?.position ?? 0; - - return ( - (recordAfterDestinationPosition + recordDestinationPosition) / 2 - ); - } else { - return ( - recordDestination?.position - - (recordDestination?.position - recordBefore?.position) / 2 - ); - } - }; - - const newPosition = computeNewPosition(destinationIndex, sourceIndex); - - return { draggedRecordId, newPosition }; - }, - [], - ); -}; diff --git a/packages/twenty-front/src/modules/object-record/record-table/record-table-body/components/RecordTableBodyDragDropContextProvider.tsx b/packages/twenty-front/src/modules/object-record/record-table/record-table-body/components/RecordTableBodyDragDropContextProvider.tsx index 4c5575fd00..aced5e0b3c 100644 --- a/packages/twenty-front/src/modules/object-record/record-table/record-table-body/components/RecordTableBodyDragDropContextProvider.tsx +++ b/packages/twenty-front/src/modules/object-record/record-table/record-table-body/components/RecordTableBodyDragDropContextProvider.tsx @@ -1,13 +1,15 @@ import { DragDropContext, DropResult } from '@hello-pangea/dnd'; import { ReactNode, useContext } from 'react'; -import { useSetRecoilState } from 'recoil'; +import { useRecoilCallback, useSetRecoilState } from 'recoil'; import { useUpdateOneRecord } from '@/object-record/hooks/useUpdateOneRecord'; +import { getDraggedRecordPosition } from '@/object-record/record-board/utils/getDraggedRecordPosition'; import { recordIndexAllRecordIdsComponentSelector } from '@/object-record/record-index/states/selectors/recordIndexAllRecordIdsComponentSelector'; +import { recordStoreFamilyState } from '@/object-record/record-store/states/recordStoreFamilyState'; import { RecordTableContext } from '@/object-record/record-table/contexts/RecordTableContext'; -import { useComputeNewRowPosition } from '@/object-record/record-table/hooks/useComputeNewRowPosition'; import { isRemoveSortingModalOpenState } from '@/object-record/record-table/states/isRemoveSortingModalOpenState'; -import { useRecoilComponentValueV2 } from '@/ui/utilities/state/component-state/hooks/useRecoilComponentValueV2'; +import { getSnapshotValue } from '@/ui/utilities/recoil-scope/utils/getSnapshotValue'; +import { useRecoilComponentCallbackStateV2 } from '@/ui/utilities/state/component-state/hooks/useRecoilComponentCallbackStateV2'; import { useGetCurrentView } from '@/views/hooks/useGetCurrentView'; import { isDefined } from '~/utils/isDefined'; @@ -22,7 +24,7 @@ export const RecordTableBodyDragDropContextProvider = ({ objectNameSingular, }); - const allRecordIds = useRecoilComponentValueV2( + const recordIndexAllRecordIdsSelector = useRecoilComponentCallbackStateV2( recordIndexAllRecordIdsComponentSelector, ); @@ -35,27 +37,75 @@ export const RecordTableBodyDragDropContextProvider = ({ isRemoveSortingModalOpenState, ); - const computeNewRowPosition = useComputeNewRowPosition(); + const handleDragEnd = useRecoilCallback( + ({ snapshot }) => + (result: DropResult) => { + if (viewSorts.length > 0) { + setIsRemoveSortingModalOpenState(true); + return; + } - const handleDragEnd = (result: DropResult) => { - if (viewSorts.length > 0) { - setIsRemoveSortingModalOpenState(true); - return; - } + if (!isDefined(result.destination)) { + throw new Error('Drop Destination is not defined'); + } - const computeResult = computeNewRowPosition(result, allRecordIds); + const allRecordIds = getSnapshotValue( + snapshot, + recordIndexAllRecordIdsSelector, + ); - if (!isDefined(computeResult)) { - return; - } + const isSourceIndexBeforeDestinationIndex = + result.source.index < result.destination.index; - updateOneRow({ - idToUpdate: computeResult.draggedRecordId, - updateOneRecordInput: { - position: computeResult.newPosition, + const recordBeforeDestinationId = + allRecordIds[ + isSourceIndexBeforeDestinationIndex + ? result.destination.index + : result.destination.index - 1 + ]; + + const recordBeforeDestination = recordBeforeDestinationId + ? snapshot + .getLoadable(recordStoreFamilyState(recordBeforeDestinationId)) + .getValue() + : null; + + const recordAfterDestinationId = + allRecordIds[ + isSourceIndexBeforeDestinationIndex + ? result.destination.index + 1 + : result.destination.index + ]; + + const recordAfterDestination = recordAfterDestinationId + ? snapshot + .getLoadable(recordStoreFamilyState(recordAfterDestinationId)) + .getValue() + : null; + + const newPosition = getDraggedRecordPosition( + recordBeforeDestination?.position, + recordAfterDestination?.position, + ); + + if (!isDefined(newPosition)) { + return; + } + + updateOneRow({ + idToUpdate: result.draggableId, + updateOneRecordInput: { + position: newPosition, + }, + }); }, - }); - }; + [ + recordIndexAllRecordIdsSelector, + setIsRemoveSortingModalOpenState, + updateOneRow, + viewSorts.length, + ], + ); return ( {children} diff --git a/packages/twenty-front/src/modules/object-record/record-table/record-table-body/components/RecordTableBodyRecordGroupDragDropContextProvider.tsx b/packages/twenty-front/src/modules/object-record/record-table/record-table-body/components/RecordTableBodyRecordGroupDragDropContextProvider.tsx index 5aaa380776..6e229447c9 100644 --- a/packages/twenty-front/src/modules/object-record/record-table/record-table-body/components/RecordTableBodyRecordGroupDragDropContextProvider.tsx +++ b/packages/twenty-front/src/modules/object-record/record-table/record-table-body/components/RecordTableBodyRecordGroupDragDropContextProvider.tsx @@ -3,10 +3,11 @@ import { ReactNode, useContext } from 'react'; import { useRecoilCallback, useSetRecoilState } from 'recoil'; import { useUpdateOneRecord } from '@/object-record/hooks/useUpdateOneRecord'; +import { getDraggedRecordPosition } from '@/object-record/record-board/utils/getDraggedRecordPosition'; import { recordGroupDefinitionFamilyState } from '@/object-record/record-group/states/recordGroupDefinitionFamilyState'; -import { recordIndexAllRecordIdsComponentSelector } from '@/object-record/record-index/states/selectors/recordIndexAllRecordIdsComponentSelector'; +import { recordIndexRecordIdsByGroupComponentFamilyState } from '@/object-record/record-index/states/recordIndexRecordIdsByGroupComponentFamilyState'; +import { recordStoreFamilyState } from '@/object-record/record-store/states/recordStoreFamilyState'; import { RecordTableContext } from '@/object-record/record-table/contexts/RecordTableContext'; -import { useComputeNewRowPosition } from '@/object-record/record-table/hooks/useComputeNewRowPosition'; import { isRemoveSortingModalOpenState } from '@/object-record/record-table/states/isRemoveSortingModalOpenState'; import { useRecoilComponentCallbackStateV2 } from '@/ui/utilities/state/component-state/hooks/useRecoilComponentCallbackStateV2'; import { getSnapshotValue } from '@/ui/utilities/state/utils/getSnapshotValue'; @@ -25,10 +26,6 @@ export const RecordTableBodyRecordGroupDragDropContextProvider = ({ objectNameSingular, }); - const recordIndexAllRecordIdsSelector = useRecoilComponentCallbackStateV2( - recordIndexAllRecordIdsComponentSelector, - ); - const { currentViewWithCombinedFiltersAndSorts } = useGetCurrentView(recordTableId); @@ -38,33 +35,34 @@ export const RecordTableBodyRecordGroupDragDropContextProvider = ({ isRemoveSortingModalOpenState, ); - const computeNewRowPosition = useComputeNewRowPosition(); + const recordIdsByGroupFamilyState = useRecoilComponentCallbackStateV2( + recordIndexRecordIdsByGroupComponentFamilyState, + ); const handleDragEnd = useRecoilCallback( ({ snapshot }) => (result: DropResult) => { - const tableAllRecordIds = getSnapshotValue( - snapshot, - recordIndexAllRecordIdsSelector, - ); + const destinationRecordGroupId = result.destination?.droppableId; - const recordGroupId = result.destination?.droppableId; + if (!isDefined(result.destination)) { + throw new Error('Drop Destination is not defined'); + } - if (!isDefined(recordGroupId)) { + if (!isDefined(destinationRecordGroupId)) { throw new Error('Record group id is not defined'); } - const recordGroup = getSnapshotValue( + const destinationRecordGroup = getSnapshotValue( snapshot, - recordGroupDefinitionFamilyState(recordGroupId), + recordGroupDefinitionFamilyState(destinationRecordGroupId), ); - if (!isDefined(recordGroup)) { + if (!isDefined(destinationRecordGroup)) { throw new Error('Record group is not defined'); } const fieldMetadata = objectMetadataItem.fields.find( - (field) => field.id === recordGroup.fieldMetadataId, + (field) => field.id === destinationRecordGroup.fieldMetadataId, ); if (!isDefined(fieldMetadata)) { @@ -76,25 +74,62 @@ export const RecordTableBodyRecordGroupDragDropContextProvider = ({ return; } - const computeResult = computeNewRowPosition(result, tableAllRecordIds); + const isSourceIndexBeforeDestinationIndexInSameGroup = + result.source.index < result.destination.index && + result.source.droppableId === result.destination.droppableId; - if (!isDefined(computeResult)) { + const destinationGroupRecordIds = getSnapshotValue( + snapshot, + recordIdsByGroupFamilyState(destinationRecordGroupId), + ); + + const recordBeforeDestinationId = + destinationGroupRecordIds[ + isSourceIndexBeforeDestinationIndexInSameGroup + ? result.destination.index + : result.destination.index - 1 + ]; + + const recordBeforeDestination = recordBeforeDestinationId + ? snapshot + .getLoadable(recordStoreFamilyState(recordBeforeDestinationId)) + .getValue() + : null; + + const recordAfterDestinationId = + destinationGroupRecordIds[ + isSourceIndexBeforeDestinationIndexInSameGroup + ? result.destination.index + 1 + : result.destination.index + ]; + + const recordAfterDestination = recordAfterDestinationId + ? snapshot + .getLoadable(recordStoreFamilyState(recordAfterDestinationId)) + .getValue() + : null; + + const newPosition = getDraggedRecordPosition( + recordBeforeDestination?.position, + recordAfterDestination?.position, + ); + + if (!isDefined(newPosition)) { return; } updateOneRow({ - idToUpdate: computeResult.draggedRecordId, + idToUpdate: result.draggableId, updateOneRecordInput: { - position: computeResult.newPosition, - [fieldMetadata.name]: recordGroup.value, + position: newPosition, + [fieldMetadata.name]: destinationRecordGroup.value, }, }); }, [ - recordIndexAllRecordIdsSelector, objectMetadataItem.fields, viewSorts.length, - computeNewRowPosition, + recordIdsByGroupFamilyState, updateOneRow, setIsRemoveSortingModalOpenState, ], diff --git a/packages/twenty-front/src/modules/object-record/record-table/record-table-row/components/RecordTablePendingRow.tsx b/packages/twenty-front/src/modules/object-record/record-table/record-table-row/components/RecordTablePendingRow.tsx index a49784e7a3..be15832390 100644 --- a/packages/twenty-front/src/modules/object-record/record-table/record-table-row/components/RecordTablePendingRow.tsx +++ b/packages/twenty-front/src/modules/object-record/record-table/record-table-row/components/RecordTablePendingRow.tsx @@ -13,7 +13,8 @@ export const RecordTablePendingRow = () => { ); diff --git a/packages/twenty-front/src/modules/object-record/record-table/record-table-row/components/RecordTableRow.tsx b/packages/twenty-front/src/modules/object-record/record-table/record-table-row/components/RecordTableRow.tsx index 382fdc2303..2b3699c896 100644 --- a/packages/twenty-front/src/modules/object-record/record-table/record-table-row/components/RecordTableRow.tsx +++ b/packages/twenty-front/src/modules/object-record/record-table/record-table-row/components/RecordTableRow.tsx @@ -7,19 +7,22 @@ import { RecordTableRowWrapper } from '@/object-record/record-table/record-table type RecordTableRowProps = { recordId: string; - rowIndex: number; + rowIndexForFocus: number; + rowIndexForDrag: number; isPendingRow?: boolean; }; export const RecordTableRow = ({ recordId, - rowIndex, + rowIndexForFocus, + rowIndexForDrag, isPendingRow, }: RecordTableRowProps) => { return ( diff --git a/packages/twenty-front/src/modules/object-record/record-table/record-table-row/components/RecordTableRowWrapper.tsx b/packages/twenty-front/src/modules/object-record/record-table/record-table-row/components/RecordTableRowWrapper.tsx index 940398243e..54e1913007 100644 --- a/packages/twenty-front/src/modules/object-record/record-table/record-table-row/components/RecordTableRowWrapper.tsx +++ b/packages/twenty-front/src/modules/object-record/record-table/record-table-row/components/RecordTableRowWrapper.tsx @@ -16,14 +16,16 @@ import { useSetRecoilComponentStateV2 } from '@/ui/utilities/state/component-sta type RecordTableRowWrapperProps = { recordId: string; - rowIndex: number; + rowIndexForFocus: number; + rowIndexForDrag: number; isPendingRow?: boolean; children: ReactNode; }; export const RecordTableRowWrapper = ({ recordId, - rowIndex, + rowIndexForFocus, + rowIndexForDrag, isPendingRow, children, }: RecordTableRowWrapperProps) => { @@ -55,7 +57,7 @@ export const RecordTableRowWrapper = ({ ); useEffect(() => { - if (rowIndex === 0) { + if (rowIndexForFocus === 0) { const tdArray = Array.from( trRef.current?.getElementsByTagName('td') ?? [], ); @@ -66,7 +68,7 @@ export const RecordTableRowWrapper = ({ setTableCellWidths(tdWidths); } - }, [trRef, rowIndex, setTableCellWidths]); + }, [trRef, rowIndexForFocus, setTableCellWidths]); // TODO: find a better way to emit this event useEffect(() => { @@ -76,7 +78,7 @@ export const RecordTableRowWrapper = ({ }, [inView, onIndexRecordsLoaded]); return ( - + {(draggableProvided, draggableSnapshot) => ( { @@ -103,7 +105,7 @@ export const RecordTableRowWrapper = ({ ( - componentState: RecoilComponentState, - componentId?: string, -) => { - const componentContext = (window as any).componentContextStateMap?.get( - componentState.key, - ); - - if (!componentContext) { - throw new Error( - `Component context for key "${componentState.key}" is not defined`, - ); - } - - const internalScopeId = useAvailableScopeIdOrThrow( - componentContext, - getScopeIdOrUndefinedFromComponentId(componentId), - ); - - return componentState.atomFamily({ - scopeId: internalScopeId, - }); -};