feat: Column organization overflow and undo/redo#2546
feat: Column organization overflow and undo/redo#2546mattrunyon merged 13 commits intodeephaven:mainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2546 +/- ##
==========================================
- Coverage 44.15% 44.04% -0.12%
==========================================
Files 763 764 +1
Lines 42906 43041 +135
Branches 11011 10866 -145
==========================================
+ Hits 18944 18956 +12
- Misses 23916 24069 +153
+ Partials 46 16 -30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| const undoBtn = screen.getByLabelText('Undo'); | ||
| await user.click(undoBtn); | ||
| expect(mockGroupHandler).toHaveBeenCalledWith([]); |
There was a problem hiding this comment.
If I'm understanding correctly, this test proves it results in an empty array if only 1 action had occurred. I'm assuming if the stack had multiple items, it would show an array containing all but the last one? If so, might be good to test this just to prove the undo doesn't always undo everything in the stack. Same for the keyboard tests.
There was a problem hiding this comment.
Yes. I'll add a test for multiple changes. The hook has a test for multiple items, but might as well test the implementation too
| } | ||
|
|
||
| onColumnHeaderGroupChanged(newGroups); | ||
| endUndoGroup(); |
There was a problem hiding this comment.
Are there any edge cases where handleGroupNameChange might not get called after startUndoGroup has been called resutling in endUndoGroup not being called?
There was a problem hiding this comment.
Good call. I found one potential if you created a new group and deleted it without ever saving.
So the only way to start the grouping is to create a new column group.
The only ways out of creating a column group are save the group or delete it. Both call endUndoGroup now
bmingles
left a comment
There was a problem hiding this comment.
General logic seems solid. I left a few questions / suggestions.
bmingles
left a comment
There was a problem hiding this comment.
General logic looks good. Left a few questions / suggestions.
| ], | ||
| [] | ||
| ); | ||
| return items |
There was a problem hiding this comment.
May not be worth changing if the depth / number of items is relatively small, but I typically like to use stack / queues for tree traversal:
Note this is mostly just to capture my thoughts on this. Not a change required for this PR.
I haven't tested, but I think this is functionally equivalent:
// DFS tree traversal to flatten a tree
function flatten<T>(items: ReadonlyTreeItems<T>): FlattenedItem<T>[] {
const result: FlattenedItem<T>[] = [];
const stack: [parentId: string | null, depth: number, item: TreeItem<T>][] =
[];
// Initialize the stack with the root items in reverse order so they can be
// popped in forward order
for (let i = items.length - 1; i >= 0; i -= 1) {
stack.push([null, 0, items[i]]);
}
let index = 0;
while (stack.length) {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const [parentId, depth, item] = stack.pop()!;
if (item.children.length) {
for (let i = item.children.length - 1; i >= 0; i -= 1) {
stack.push([item.id, depth + 1, item.children[i]]);
}
}
result.push({ ...item, depth, parentId, index });
index += 1;
}
return result;
}There was a problem hiding this comment.
Depth should be < 10 and almost certainly like 2 at most. Deeply nested groups can be hard to use and I'd be surprised if people have more than a couple layers of grouping on a table
bmingles
left a comment
There was a problem hiding this comment.
Left a comment about tree traversal, but not a blocker for the PR
Adds column order and visibilty overflow menu with show hidden columns (for the visibility menu only) and undo/redo.
Moves in the menu or on the grid add to the undo/redo stack.
Hiding/unhiding adds to the undo/redo stack.
Creating a new group will add to the stack as a whole action when it is named (moving columns + naming the group)
Unchecking the "Show hidden columns" option will hide the hidden column/group entries in the menu. The setting is reset on menu open.