-
Notifications
You must be signed in to change notification settings - Fork 33
feat: Column organization overflow and undo/redo #2546
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.
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
base: main
Are you sure you want to change the base?
feat: Column organization overflow and undo/redo #2546
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
packages/iris-grid/src/sidebar/visibility-ordering-builder/VisibilityOrderingBuilder.test.tsx
Show resolved
Hide resolved
|
||
const { columnHeaderGroups, onColumnHeaderGroupChanged } = this.props; | ||
// Clean up unnamed groups on unmount | ||
const newGroups = columnHeaderGroups.filter(group => !group.isNew); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually newGroups? Seems it should be oldGroups or something based on the !group.isNew filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya I think I just copied the naming out of the inner component where this logic was handled before. But that was causing unnecessary state changes when the group was named because it would replace the group item and trigger the "delete on unmount" behavior it had
} | ||
|
||
onColumnHeaderGroupChanged(newGroups); | ||
endUndoGroup(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
packages/iris-grid/src/sidebar/visibility-ordering-builder/VisibilityOrderingBuilder.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General logic seems solid. I left a few questions / suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General logic looks good. Left a few questions / suggestions.
], | ||
[] | ||
); | ||
return items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.