Fix bug reoder on table in view groups mode #8894
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Error: Error in underlying LLM call. Please try again.
💡 (4/5) You can add custom instructions or style guidelines for the bot here!
5 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -76,24 +76,32 @@ export const RecordTableBodyRecordGroupDragDropContextProvider = ({ | |||
| return; | |||
| } | |||
|
|
|||
| const computeResult = computeNewRowPosition(result, tableAllRecordIds); | |||
| const desinationRecordGroupRecordIds = getSnapshotValue( | |||
There was a problem hiding this comment.
syntax: 'desinationRecordGroupRecordIds' is misspelled
| if (!isDefined(destinationRecordGroupId)) { | ||
| throw new Error('Record group id is not defined'); | ||
| } |
There was a problem hiding this comment.
style: Throwing an error here might be too aggressive - consider gracefully handling the case where drag operation is cancelled
| return; | ||
| } | ||
|
|
||
| console.log(result.destination.index); |
There was a problem hiding this comment.
style: Remove debug console.log statement
| : null; | ||
|
|
||
| if (destinationIndex === 0) { | ||
| const newPosition = recordDestination?.position - 1; |
There was a problem hiding this comment.
logic: recordDestination could be null, leading to undefined position and NaN result
| const newPosition = | ||
| (recordBeforeDestination?.position + | ||
| recordAfterDestination?.position) / | ||
| 2; |
There was a problem hiding this comment.
logic: Both records could be null or have undefined positions, causing NaN. Need null checks before calculation
|
|
||
| const recordBeforeDestinationId = | ||
| recordIds[recordBeforeDestinationIndex]; | ||
| console.log(recordBeforeDestinationId); |
There was a problem hiding this comment.
style: Remove debug console.log statement
| : destinationIndex; | ||
|
|
||
| const recordAfterDestinationId = recordIds[recordAfterDestinationIndex]; | ||
| console.log(recordAfterDestinationId); |
There was a problem hiding this comment.
style: Remove debug console.log statement
| @@ -15,7 +15,8 @@ export const RecordTableNoRecordGroupRows = () => { | |||
| <RecordTableRow | |||
| key={recordId} | |||
| recordId={recordId} | |||
| rowIndex={rowIndex} | |||
| rowIndexForFocus={rowIndex} | |||
There was a problem hiding this comment.
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
| @@ -1,90 +0,0 @@ | |||
| import { DropResult } from '@hello-pangea/dnd'; | |||
There was a problem hiding this comment.
removing this hook as we should leave recordBoard, recordTable, recordTableWithGroups implement their way of selecting the right records + we already have getDraggedRecordPosition helper
| position: computeResult.newPosition, | ||
| const recordBeforeDestinationId = | ||
| allRecordIds[ | ||
| isSourceIndexBeforeDestinationIndex |
There was a problem hiding this comment.
this is subtle: if the previous record is before or after, we need to shift the records before and after by 1
|
|
||
| const recordGroupId = result.destination?.droppableId; | ||
| if (!isDefined(result.destination)) { |
There was a problem hiding this comment.
same logic but in case of viewGroups
Fixes #8403
Added comments on the PR to explain changes