feat: Column organization overflow and undo/redo #2546
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2546 +/- ##
==========================================
+ Coverage 43.91% 44.01% +0.09%
==========================================
Files 763 764 +1
Lines 42874 43016 +142
Branches 11005 11040 +35
==========================================
+ Hits 18827 18932 +105
- Misses 24032 24038 +6
- Partials 15 46 +31
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:
|
| [class*='spectrum-Menu'] { | ||
| kbd { | ||
| // Unsetting bootstrap overrides | ||
| padding: unset; |
There was a problem hiding this comment.
what's the reason for this one being unset vs the rest all being revert?
| * @returns The new reordered items | ||
| */ | ||
| static moveItem( | ||
| static moveItem<T extends readonly MoveOperation[]>( |
There was a problem hiding this comment.
What's the use case for T extending MoveOperation[]?
There was a problem hiding this comment.
Looking at this, I think these will be cleaner if you do
static moveItem<T extends MoveOperation>(
from: VisibleIndex,
to: VisibleIndex,
oldMovedItems: readonly T[]
): readonly T[] {
)Then you don't need any as unknown as T assertions. You'd only need to assert new item creation e.g. movedItems.push({ from, to } as T);. I also think this is more accurate, since it's the array items that might be subtypes and not the entire readonly array. It will require updating all 3 moveXXX functions, but seemed to work with what I tried.
|
|
||
| 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.
| const hiddenColumns = [1, 3, 5, 7, 9]; | ||
| render(<BuilderWithStateManagement hiddenColumns={hiddenColumns} />); | ||
|
|
||
| expect(screen.getAllByText(COLUMN_PREFIX, { exact: false }).length).toBe(10); |
There was a problem hiding this comment.
Curious why there are 10 items before showing and 5 after?
|
|
||
| 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.
Is this actually newGroups? Seems it should be oldGroups or something based on the !group.isNew filter.
| } | ||
|
|
||
| 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?
| } | ||
| } | ||
|
|
||
| // The forwardRef is for a hacky unit test to check the |
There was a problem hiding this comment.
I don't see forwardRef apis used here. I assume this is conceptually referring to __testRef ?
| * @returns The new reordered items | ||
| */ | ||
| static moveRange( | ||
| static moveRange<T extends readonly MoveOperation[]>( |
| } | ||
|
|
||
| static moveItemOrRange( | ||
| static moveItemOrRange<T extends readonly MoveOperation[]>( |
| // undo/redo will pass already parsed groups | ||
| // Parsing them again causes a loop with undo/redo that makes it unusable | ||
| columnHeaderGroups: | ||
| columnHeaderGroups.every(isColumnHeaderGroup) && |
There was a problem hiding this comment.
micro optimization would be for both checks to happen in the predicate instead of 2 every calls.
| @@ -0,0 +1,87 @@ | |||
| import { renderHook, act } from '@testing-library/react'; | |||
There was a problem hiding this comment.
Kind of confused why this wouldn't fail the tests.
suggestion
import { renderHook, act } from '@testing-library/react-hooks';
There was a problem hiding this comment.
oh, after npm install this works. Wonder if this was a recent addition to @testing-library/react exports
|
|
||
| return { | ||
| state: newState, | ||
| undoStack, |
There was a problem hiding this comment.
Is there any risk using a mutated undoStack instead of undoStack: [...undoStack]?
2 participants
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.