Skip to content
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

Fix bug reoder on table in view groups mode #8894

Merged
merged 4 commits into from
Dec 6, 2024
Merged

Fix bug reoder on table in view groups mode #8894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ab7037c
Fix bug reoder on table in view groups mode
charlesBochet Dec 5, 2024
a0c5e10
Re-order fix
charlesBochet Dec 5, 2024
8b06f01
Fix
charlesBochet Dec 6, 2024
79b2987
Fix
charlesBochet Dec 6, 2024
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
3 changes: 2 additions & 1 deletion ...-front/src/modules/object-record/record-table/components/RecordTableNoRecordGroupRows.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ export const RecordTableNoRecordGroupRows = () => {
<RecordTableRow
key={recordId}
recordId={recordId}
rowIndex={rowIndex}
rowIndexForFocus={rowIndex}
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm introducing two rowIndexes:

  • one is used to handle focus, the other to give an index for drag

Right now the index used in focus / cell opening is handled at table level so it's a global index, the index use drag is at group level. I think long term we want to have the index for focus / cell opening at group level too and handle the jump between groups properly

I prefer introducing these two naming ForFocus and ForDrag because it's clear when to use which one in underlying components. Alternatively I could have introduced rowIndexInGroup and rowIndexInTable but this will make underlying components know about recordGroups and make if conditions which I think we should avoid to reduce complexity

rowIndexForDrag={rowIndex}
/>
);
})}
Expand Down
5 changes: 3 additions & 2 deletions ...ty-front/src/modules/object-record/record-table/components/RecordTableRecordGroupRows.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const RecordTableRecordGroupRows = () => {

return (
isRecordGroupTableSectionToggled &&
recordIdsByGroup.map((recordId) => {
recordIdsByGroup.map((recordId, rowIndexInGroup) => {
const rowIndex = rowIndexMap.get(recordId);

if (!isDefined(rowIndex)) {
Expand All @@ -43,7 +43,8 @@ export const RecordTableRecordGroupRows = () => {
<RecordTableRow
key={recordId}
recordId={recordId}
rowIndex={rowIndex}
rowIndexForFocus={rowIndex}
rowIndexForDrag={rowIndexInGroup}
isPendingRow={!isRecordGroupTableSectionToggled}
/>
);
Expand Down
90 changes: 0 additions & 90 deletions ...ges/twenty-front/src/modules/object-record/record-table/hooks/useComputeNewRowPosition.ts
View file
Open in desktop

This file was deleted.

90 changes: 70 additions & 20 deletions ...cord/record-table/record-table-body/components/RecordTableBodyDragDropContextProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -22,7 +24,7 @@ export const RecordTableBodyDragDropContextProvider = ({
objectNameSingular,
});

const allRecordIds = useRecoilComponentValueV2(
const recordIndexAllRecordIdsSelector = useRecoilComponentCallbackStateV2(
recordIndexAllRecordIdsComponentSelector,
);

Expand All @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

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

this is subtle: if the previous record is before or after, we need to shift the records before and after by 1

? 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 (
<DragDropContext onDragEnd={handleDragEnd}>{children}</DragDropContext>
Expand Down
83 changes: 59 additions & 24 deletions ...-table/record-table-body/components/RecordTableBodyRecordGroupDragDropContextProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -25,10 +26,6 @@ export const RecordTableBodyRecordGroupDragDropContextProvider = ({
objectNameSingular,
});

const recordIndexAllRecordIdsSelector = useRecoilComponentCallbackStateV2(
recordIndexAllRecordIdsComponentSelector,
);

const { currentViewWithCombinedFiltersAndSorts } =
useGetCurrentView(recordTableId);

Expand All @@ -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)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

same logic but in case of viewGroups

throw new Error('Drop Destination is not defined');
}

if (!isDefined(recordGroupId)) {
if (!isDefined(destinationRecordGroupId)) {
throw new Error('Record group id is not defined');
}
Comment on lines +51 to 53
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Throwing an error here might be too aggressive - consider gracefully handling the case where drag operation is cancelled


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)) {
Expand All @@ -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;

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(computeResult)) {
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,
],
Expand Down
3 changes: 2 additions & 1 deletion .../modules/object-record/record-table/record-table-row/components/RecordTablePendingRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ export const RecordTablePendingRow = () => {
<RecordTableRow
key={pendingRecordId}
recordId={pendingRecordId}
rowIndex={-1}
rowIndexForDrag={-1}
rowIndexForFocus={-1}
isPendingRow
/>
);
Expand Down
Loading
Loading