-
Notifications
You must be signed in to change notification settings - Fork 348
Freeze right columns #1059
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?
Freeze right columns #1059
Conversation
17d0bf2
to
f378e2e
Compare
@@ -70,7 +70,7 @@ export interface DataGridProps { | |||
|
|||
readonly accessibilityHeight: number; | |||
|
|||
readonly freezeColumns: number; | |||
readonly freezeColumns: number | [left: number, right: number]; |
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.
For a stricter type definition, I suggest using the readonly
flag here.
readonly freezeColumns: number | [left: number, right: number]; | |
readonly freezeColumns: number | readonly [left: number, right: number]; |
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.
Added, thanks for the suggestion!
f378e2e
to
f38bcec
Compare
Pushed a few updates and rebased with
cc: @citizensas |
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.
Pull Request Overview
Adds full support for freezing columns on the right side in addition to the existing left‐side freeze functionality.
- Expanded the
freezeColumns
prop from a singlenumber
to a unionnumber | [left, right]
- Updated nearly all core layout and render functions (
getEffectiveColumns
,walkColumns
, blitting, clipping, bounds calculations, etc.) to handle both left and right sticky columns - Extended tests and documentation examples to cover the new right‐side freeze behavior
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
packages/source/src/use-collapsing-groups.ts | Limit group span calculations to the scrollable central columns |
packages/core/test/render-state-provider.test.ts | Updated setWindow calls to include the new columnsLength arg |
packages/core/test/image-window-loader.test.ts | Extended loader tests for the added columnsLength parameter |
packages/core/test/data-grid.test.tsx | Added UI test for freezing with both left and right columns |
packages/core/test/data-editor.test.tsx | Added test covering visible‐region change including right freeze |
packages/core/src/internal/scrolling-data-grid/scrolling-data-grid.tsx | Scrolling logic now uses left freeze count and ignores right side |
packages/core/src/internal/data-grid/render/draw-grid-arg.ts | Expanded freezeColumns in DrawGridArg to accept [left,right] |
packages/core/src/internal/data-grid/render/data-grid.render.rings.ts | Split highlight regions into left/center/right panes |
packages/core/src/internal/data-grid/render/data-grid.render.walk.ts | Column walking now handles right‐side frozen columns |
packages/core/src/internal/data-grid/render/data-grid-render.ts | Central render updated to support right‐side sticky columns |
packages/core/src/internal/data-grid/render/data-grid-render.lines.ts | Grid line drawing now draws right‐side sticky boundaries |
packages/core/src/internal/data-grid/render/data-grid-render.header.ts | Header clipping/hover now respects right‐side freeze |
packages/core/src/internal/data-grid/render/data-grid-render.cells.ts | Cell draw logic updated for right‐side sticky widths |
packages/core/src/internal/data-grid/render/data-grid-render.blit.ts | Blit logic adjusted to copy right sticky region as well |
packages/core/src/internal/data-grid/render/data-grid-lib.ts | useMappedColumns now marks right‐side sticky positions |
packages/core/src/internal/data-grid/image-window-loader-interface.ts | Loader interface now takes freezeCols as `number |
packages/core/src/internal/data-grid/data-grid.tsx | DataGridProps and event logic updated for tuple freeze |
packages/core/src/docs/examples/freeze-columns.stories.tsx | Story props split into freezeLeftColumns /freezeRightColumns |
packages/core/src/data-editor/data-editor.tsx | DataEditorImpl computations now include right‐side freeze counts |
packages/core/src/common/render-state-provider.ts | Window tracker now handles tuple freeze and track total columns |
Comments suppressed due to low confidence (3)
packages/core/src/common/render-state-provider.ts:39
- [nitpick] Consider renaming the class property
freezeCols
tofreezeColumns
to match the prop name and improve consistency across the codebase.
public freezeCols: number | readonly [number, number] = 0;
packages/core/src/internal/data-grid/image-window-loader-interface.ts:11
- [nitpick] Rename the
freezeCols
parameter in thesetWindow
interface tofreezeColumns
for consistency with the primary API.
): void;
packages/core/test/data-grid.test.tsx:396
- Add targeted unit tests for
getColumnIndexForX
andgetEffectiveColumns
to validate both left-only and right-only freeze scenarios (e.g.[0, n]
,[n, 0]
), including edge cases.
test("Freeze column simple check with trailing", () => {
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.
Overall, LGTM. Would update API.md and look at the possible off by one, but merge-able! Thanks 😎
frozenLeftWidth += columns[i].width; | ||
} | ||
let frozenRightWidth = 0; | ||
for (let i = columns.length - 1; i >= columns.length - 1 - freezeRightColumns; i--) { |
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.
Should this be i >= columns.length - freezeRightColumns
? Possible off-by-one error.
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.
Yep, changed, thanks for the catch!
const freezeLeftColumns = typeof freezeColumns === "number" ? freezeColumns : freezeColumns[0]; | ||
const freezeRightColumns = typeof freezeColumns === "number" ? 0 : freezeColumns[1]; |
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.
Might be worth extracting into helper util if we use it so many times.
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.
Done, thank you
const gridSelection = gridSelectionIn ?? gridSelectionInner; | ||
|
||
const spans = React.useMemo(() => { | ||
const result: [number, number][] = []; | ||
let current: [number, number] = [-1, -1]; | ||
let lastGroup: string | undefined; | ||
for (let i = freezeColumns; i < columnsIn.length; i++) { | ||
for (let i = freezeColumnsLeft as number; i < columnsIn.length - freezeColumnsRight; i++) { |
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.
Would cast as number above ^
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.
Changed, thank you
0529640
to
67a8c55
Compare
This MR adds support for freezing columns on the right side.
It used draft MR #973 as a base, with major fixes and improvements:
This addresses the right-side freeze column feature requested in multiple issues: #469, #522, #919, and #957.
The API remains backward compatible, following the proposal here by @jassmith: