Persistence of view column ordering. #1541
Conversation
…ng. Add local state to keep track of column order.
|
Added debouncing to keep requests to a reasonable level. But there was still a problem with the rendering when reordering the columns in the redux state. So instead I chose to keep track of the new column order in the component. Of course when the page is reloaded, the columns from the redux state (provided as a prop to the ViewDataTable) are again in the correct order according to the new order. Screencast.from.2023-09-19.22-32-14.webm |
There was a problem hiding this comment.
This is a tricky one to crack!
I have began testing it to see how it works, and I am having some unexpected behaviour: the first time I move a column it works fine, but when dropping the second column the first moves back to it's original place. Not sure what causes it, but I think it should be fixed before further review.
reorder.columns.mp4
|
I believe I know what's wrong. The "origin" that the new order is calculated from is the order of the columns in the redux state, and I don't think it checks for the order in the local state when it creates the new order array, so it's reset when a second column reordering is made. This should be a simple fix, I will get to it tomorrow. |
|
The bug above has been fixed and the typing error that the CI caught. |
There was a problem hiding this comment.
I get this error when I try to move a column (in a view/list that has many different kinds of columns).
Also, it seems like these changes have introduced another bug, which is that when I add a column, it content of the view does not reload to reflect the new column. I have to refresh the page to see the newly added column.
…n-order-peristence
…a is stale as well as row data when adding a column, so that both will be reloaded.
|
Reimplemented using redux store. Everything seems to work now! |
| colList.items = colList.items.concat([ | ||
| remoteItem(column.id, { data: column }), | ||
| ]); | ||
| colList.isStale = true; |
There was a problem hiding this comment.
Why is this necessary? Adding a column was working before this PR, right? So why not keep the old logic for just concatenating it to the end of the list of columns?
There was a problem hiding this comment.
I tried changing back to the previous code and it still worked fine. So if you can please explain why you found this change to be necessary I'd appreciate that.
src/features/views/store.ts
Outdated
| if (col) { | ||
| return col; | ||
| } else { | ||
| throw new Error('Could not find column!'); |
There was a problem hiding this comment.
This should never happen, but if it does, it will now crash the app. Instead of throwing an error and crashing, wouldn't it be better to just mark the column list as stale and force it to reload?
| // Re-arrange columns of data-rows | ||
| const rowList = state.rowsByViewId[viewId]; | ||
| if (rowList) { | ||
| const newRowListItems = rowList.items.map((row) => { | ||
| if (row.data) { | ||
| return { | ||
| ...row, | ||
| data: { | ||
| content: columnOrder.map((colId) => { | ||
| const idx = colList.items.findIndex( | ||
| (col) => col.id == colId | ||
| )!; | ||
| return row.data?.content[idx]; | ||
| }), | ||
| id: row.data.id, | ||
| }, | ||
| }; | ||
| } else { | ||
| return row; | ||
| } | ||
| }); | ||
| state.columnsByViewId[viewId].items = newColListItems; | ||
| state.rowsByViewId[viewId].items = newRowListItems; | ||
| } | ||
| } |
There was a problem hiding this comment.
Beautiful! This is a big improvement over Gen2 where the data had to be reloaded after reordering columns. Nice work!
|
The colList isStale is because I discovered that if you have the view in
two browser tabs, add a tab in one tab and then add a tab in the other, it
will fech more columns for the data than the number of columns in the
header. Very specific I know, but I figured there was no harm in refreshing
the columns when you were already refreshing the data.
Good idea about the isStale on error, I will fix that.
Den lör 30 sep. 2023 06:37Richard Olsson ***@***.***> skrev:
… ***@***.**** requested changes on this pull request.
Nice work on the new redux store logic! I only question two minor parts of
it. If you can respond to this asap that would be great, so that we can get
this in the release this weekend.
------------------------------
In src/features/views/store.ts
<#1541 (comment)>
:
> const colList = state.columnsByViewId[viewId];
if (colList) {
- colList.items = colList.items.concat([
- remoteItem(column.id, { data: column }),
- ]);
+ colList.isStale = true;
Why is this necessary? Adding a column was working before this PR, right?
So why not keep the old logic for just concatenating it to the end of the
list of columns?
------------------------------
In src/features/views/store.ts
<#1541 (comment)>
:
> @@ -180,6 +178,46 @@ const viewsSlice = createSlice({
}
}
},
+ columnOrderUpdated: (state, action: PayloadAction<[number, number[]]>) => {
+ const [viewId, columnOrder] = action.payload;
+ // Re-arrange columns
+ const colList = state.columnsByViewId[viewId];
+ if (colList) {
+ const newColListItems = columnOrder.map((colId) => {
+ const col = colList.items.find((col) => col.id == colId);
+ if (col) {
+ return col;
+ } else {
+ throw new Error('Could not find column!');
This should never happen, but if it does, it will now crash the app.
Instead of throwing an error and crashing, wouldn't it be better to just
mark the column list as stale and force it to reload?
------------------------------
In src/features/views/store.ts
<#1541 (comment)>
:
> + // Re-arrange columns of data-rows
+ const rowList = state.rowsByViewId[viewId];
+ if (rowList) {
+ const newRowListItems = rowList.items.map((row) => {
+ if (row.data) {
+ return {
+ ...row,
+ data: {
+ content: columnOrder.map((colId) => {
+ const idx = colList.items.findIndex(
+ (col) => col.id == colId
+ )!;
+ return row.data?.content[idx];
+ }),
+ id: row.data.id,
+ },
+ };
+ } else {
+ return row;
+ }
+ });
+ state.columnsByViewId[viewId].items = newColListItems;
+ state.rowsByViewId[viewId].items = newRowListItems;
+ }
+ }
Beautiful! This is a big improvement over Gen2 where the data had to be
reloaded after reordering columns. Nice work!
------------------------------
In src/features/views/store.ts
<#1541 (comment)>
:
> const colList = state.columnsByViewId[viewId];
if (colList) {
- colList.items = colList.items.concat([
- remoteItem(column.id, { data: column }),
- ]);
+ colList.isStale = true;
I tried changing back to the previous code and it still worked fine. So if
you can please explain why you found this change to be necessary I'd
appreciate that.
—
Reply to this email directly, view it on GitHub
<#1541 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADSE3O7ODZYU277JNHWZ3R3X46O2FANCNFSM6AAAAAA45IS2YI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I accept the motivation behind I will push a fix to the error handling, and then proceed and merge. |
richardolsson
dismissed
ziggabyte’s stale review
As far as I can tell the issues have been fixed.
Description
This PR adds persistence to column ordering by PATCHing the column_order of the view/list.
Screenshots
Screencast.from.2023-09-18.22-26-31.webm
Changes
people/views/{view_id}/column_orderevery time a column is moved.Notes to reviewer
Originally, the column order was changed in the state as well, but this resulted in problems with rendering (see mui/mui-x#5704), so now the redux state is not updated. This also means the bug where sorting resets the order persists. Not sure how to fix this issue.
Additionally, debouncing could be a good thing to add, this is because it seems there is no event for when the column dragging is finished, instead it's fired on each step, so if you're dragging the column multiple columns over, this will update the column once for every step. This seems unnecessary and could cause a race condition if the events are fired in fast succession and something causes the first request to be slower than the second.
Related issues
Resolves #1039