add support for bulk deletion of rows #1291
Conversation
|
@il3ven is attempting to deploy a commit to the Rowy Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
|
Hey @il3ven, we also have a field type called Array-subtable. Although the view appears as a table, the internal workings are a bit different. The entire array subtable refers to a single field in a document, which is an array of maps. This is an edge case that is not handled here. Screen.Recording.2023-06-21.at.12.59.32.PM.mov |
|
@il3ven tested this and it works well overall. There is just one slight issue, when you Screen.Recording.2023-06-26.at.2.43.14.pm.mov |
|
I'll try to fix all these issue by Friday. Sorry for the delay, really busy with other stuff. |
|
I have pushed three commits addressing the comments. eef15acThis commit fixes the freeze issue reported by @harinij. 8d81c36I found a new bug. The reordering of rows was not working as expected. Fixed it. dc972d7This commit addresses the comment by @iamanishroy. The solution for now is to disable row selection for array sub table. Before explaining the reason for disabling row selection let me explain the cause for the reported bug. Cause for the array sub table bugWe have the following logic for generating row ID. const getRowId = (row: TableRow) => row._rowy_ref.path || row._rowy_ref.id;All rows in case of array sub table have the same Giving unique row ID in case of array sub tableWe can have the following logic to make row IDs unique. const getRowId = (row: TableRow) =>
row._rowy_ref.path + row._rowy_ref.arrayTableData?.index
? `.${row._rowy_ref.arrayTableData?.index}`
: "";This fixes the bug shown in the video above. However, there is another bug that is the bulk delete functionality doesn't work. I have explained it below. Why can't we bulk delete rows in case of array sub table?Suppose we have three items in our array that is const [ _deleteRowDb ] = useAtom(_deleteRowDbAtom)
// This works as expected. The new array in DB is [a, c].
_deleteRowDb(path, {index: 1})
// This does not work. As there is no item at index 2.
_deleteRowDb(path, {index: 2}) The above explanation is simplified for the purpose of this explanation. Solutions for bulk delete in case of array sub tableThe best solution would be to ditch array sub table. The sub table should be a collection. We'll also be able to reuse existing code. The other solution can be to assign unique IDs to each item in the array. The ID can be autogenerated. Instead of deleting item by index we can center our logic to delete item by ID. Due to the above problem in deletion of rows. I feel it is best to disable row selection for now and implement a fix in a separate PR. |
|
Hi @il3ven thanks for the detailed explanation and updates. It appears that this PR has build errors, screenshot below - that would need further attention. |
|
@harinij fixed the build error. |
Row selection has been disabled because bulk delete functionality cannot work for array sub table according to the current implementation. Bulk delete problem: Suppose we have an array with values `[a, b, c]`. Suppose, the indexes to be removed are [1, 2]. The UI asks firebase function to remove index 1. After 1, the UI asks index 2 to be deleted but index 2 doesn't exist. After deletion of index 1, the array is `[a, c]`.
`enableRowSelection` was introduced in the last commit. As of this commit row selection is enabled for TablePage and SubTablePage.
If there are 1000 rows and we have rendered 100 then select all will only select 100. In this case, technically all rows in react-table are selected but we should not show the checked state. It leads to bad UX. Indermediate would be better. Also, split the additions of row selection column into a new useMemo. This ensures that if a row is selected we are only re-calculating the row selection column and not all.
Due to wrong CSS we were not able to see the icon.
|
@shamsmosowi I have fixed two issues. Kindly review and merge.
Recording.2023-08-24.010646.mp4 |
This PR adds support for bulk deletion of rows by implementing a checkbox next to each row. The implementation is based on the example provided by react-table.
Needs more attention
<LoadedRowStatus />component doesn't update the total number of rows. I request someone with more experience of the codebase to look into it._rowy_ref.pathand the atom delete function accepts a path. In case the id is not equal to_rowy_ref.paththe implementation will fail. I think it is not a point of worry because id should always be equal to_rowy_ref.path. Please correct me if I am wrong.Note: Check the review comments for context about the decisions I have made.
/claim #809
Closes #809
2023-06-12.025948.mp4