selectedPosition -> activePosition, column/row iterators#3979
selectedPosition -> activePosition, column/row iterators#3979
selectedPosition -> activePosition, column/row iterators#3979Conversation
nstepien
changed the title
selectedPosition -> activePosition`, column/row iteratorsselectedPosition -> activePosition, column/row iterators
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3979 +/- ##
==========================================
- Coverage 97.53% 97.51% -0.03%
==========================================
Files 36 36
Lines 1463 1449 -14
Branches 472 457 -15
==========================================
- Hits 1427 1413 -14
Misses 36 36
🚀 New features to boost your workflow:
|
| }; | ||
|
|
||
| for (const column of colSpanColumns) { | ||
| function* iterateOverRowsForColSpanArgs(): Generator<ColSpanArgs<R, SR>> { |
There was a problem hiding this comment.
Refactored to use a generator -> single loop -> dedupe lots of stuff and can return/break in that loop (nested loops didn't break the outer loop)
| break; | ||
| } | ||
| for (const column of colSpanColumns) { | ||
| if (column.frozen) continue; |
There was a problem hiding this comment.
I've added this check, since all frozen columns are displayed then we don't need to check whether they affect startIdx
| const viewportColumns: CalculatedColumn<R, SR>[] = []; | ||
| for (let colIdx = 0; colIdx <= colOverscanEndIdx; colIdx++) { | ||
| const column = columns[colIdx]; | ||
| const iterateOverViewportColumns = useCallback<IterateOverViewportColumns<R, SR>>( |
There was a problem hiding this comment.
Reusable generator to iterate over viewport columns.
Unlike the previous implementation, we do not iterate over columns between lastFrozenColumnIndex and startIdx, which may be good if we have A LOT of columns, though unlikely.
It also yields the active column, as necessary, without doing
[...columns, activeColumn],
or columns.toSpliced(index, 0, activeColumn),
or [...columns.slice(...), activeColumns, ...columns.slice(...)],
which always left a bad taste in my mouth.
| [startIdx, colOverscanEndIdx, columns, lastFrozenColumnIndex] | ||
| ); | ||
|
|
||
| const iterateOverViewportColumnsForRow = useCallback<IterateOverViewportColumnsForRow<R, SR>>( |
There was a problem hiding this comment.
Rows now use this, instead of passing an array.
It helps deduplicate colSpan handling,
the col span args is created only once per row now, instead of per column.
As a nice bonus don't pass lastFrozenColumnIndex to rows anymore.
The only "negative" is that now each row will iterate through iterateOverViewportColumns, instead of a straight for...of columnsArray but its implementation is so simple it should be fine.
| [iterateOverViewportColumns, lastFrozenColumnIndex] | ||
| ); | ||
|
|
||
| const iterateOverViewportColumnsForRowOutsideOfViewport = useCallback< |
There was a problem hiding this comment.
If the row is outside the viewport, we use this iterator instead.
If a column is active, we render this column only, just like before.
If no columns are active, i.e. the row is active, then we render no columns at all, instead of rendering all the columns like in the previous implementation!
|
|
||
| const [selectedPosition, setSelectedPosition] = useState( | ||
| (): SelectCellState | EditCellState<R> => ({ idx: -1, rowIdx: minRowIdx - 1, mode: 'SELECT' }) | ||
| const [activePosition, setActivePosition] = useState<ActiveCellState | EditCellState<R>>( |
There was a problem hiding this comment.
What a huge PR just for this change 🙃
| * Viewport: any valid position in the grid outside of header rows and summary rows | ||
| * Row selection is only allowed in TreeDataGrid | ||
| */ | ||
| function validatePosition({ idx, rowIdx }: Position) { |
There was a problem hiding this comment.
I've replaced all the individual position check utils with validatePosition.
It takes care to handle active/viewport bounds, and allows row selection only in TreeDataGrid.
Hopefully this is less confusing than trying to figure out which util to use, and if we need to use multiple utils to validate both row and column coordinates.
It's all boolean checks so it'll be fast.
| onActivePositionChange({ | ||
| rowIdx: position.rowIdx, | ||
| row: isRowIdxWithinViewportBounds(position.rowIdx) ? rows[position.rowIdx] : undefined, | ||
| row: rows[position.rowIdx], |
There was a problem hiding this comment.
Added the undefined type here, no need for the check
| selectedColumn, | ||
| ...viewportColumns.slice(lastFrozenColumnIndex + 1) | ||
| ]; | ||
| function* iterateOverViewportRowIdx() { |
There was a problem hiding this comment.
Added this generator to elegantly iterate over viewport rows.
We don't fudge around with row indexes anymore, less maths, more booleans, much simpler IMO.
| ); | ||
| } | ||
|
|
||
| function getRowViewportColumns(rowIdx: number) { |
There was a problem hiding this comment.
This has been replaced by iterateOverViewportColumnsForRow & iterateOverViewportColumnsForRowOutsideOfViewport
2 participants
To help differentiate between "selected rows" (rows that get selected via checkbox),
and "selected rows/cells/position" (rows or cells that are focused, or remain active after blurring),
I've done the work to rename "selected rows/cells/position" to "active position".
While working on it I also tried to refactor how to iterate over rows,
which lead me to refactor how we iterate over columns as well.