Skip to content

Add frozen: 'start' | 'end' for end-edge column pinning#4034

Open
PaloSP wants to merge 3 commits into
Comcast:mainfrom
PaloSP:feat/end-frozen-columns
Open

Add frozen: 'start' | 'end' for end-edge column pinning#4034
PaloSP wants to merge 3 commits into
Comcast:mainfrom
PaloSP:feat/end-frozen-columns

Conversation

@PaloSP
Copy link
Copy Markdown

@PaloSP PaloSP commented Apr 24, 2026

Adds right-edge column pinning — long-standing ask (#2873, #3671).

  • Column.frozen widens to boolean | 'start' | 'end'. true stays an alias for 'start' (backwards compatible).
  • 'end' pins to the inline-end edge; symmetric infra: --rdg-frozen-end-${idx} CSS var, rdg-cell-frozen-end class, end-shadow + top/bottom-summary variants, logical properties throughout.

Credit to @robert-luoqing for #3671 which informed the approach. API per @amanmahajan7 and @nstepien's 'start' | 'end' refinement.

One check: "we want to prevent the shadow logic" (#3671 2026-04-21) — I interpreted as "preserve" and mirrored the start-shadow symmetrically. If the intent was otherwise, happy to drop it.

Tests: new test/browser/column/frozenEnd.test.ts + additions to virtualization.test.ts and direction.test.ts. Demo updated in CommonFeatures.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 24, 2026

CLA assistant check
All committers have signed the CLA.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 24, 2026

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.53%. Comparing base (766a94e) to head (e9b24ff).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4034      +/-   ##
==========================================
+ Coverage   97.48%   97.53%   +0.05%     
==========================================
  Files          38       38              
  Lines        1552     1584      +32     
  Branches      498      511      +13     
==========================================
+ Hits         1513     1545      +32     
  Misses         39       39              
Files with missing lines Coverage Δ
src/DataGrid.tsx 98.86% <100.00%> (+0.01%) ⬆️
src/GroupRow.tsx 100.00% <100.00%> (ø)
src/HeaderRow.tsx 100.00% <ø> (ø)
src/hooks/useCalculatedColumns.ts 100.00% <100.00%> (ø)
src/hooks/useViewportColumns.ts 100.00% <100.00%> (ø)
src/utils/activePositionUtils.ts 98.92% <100.00%> (ø)
src/utils/colSpanUtils.ts 100.00% <100.00%> (ø)
src/utils/index.ts 92.30% <100.00%> (+1.39%) ⬆️
src/utils/styleUtils.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Collaborator

@amanmahajan7 amanmahajan7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR. A few quick comments. I can do a detailed review in 2 weeks (on a vacation)

Comment thread src/hooks/useCalculatedColumns.ts Outdated
}

const frozen = rawColumn.frozen ?? false;
const frozen: boolean | 'start' | 'end' = rawColumn.frozen ?? false;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a type and reuse it

Comment thread src/style/cell.ts Outdated

export const cellFrozenClassname = `rdg-cell-frozen ${cellFrozen}`;

export const cellFrozenEnd = css`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can reuse the cellFrozen class

Comment thread src/style/core.ts Outdated
`;

// Add shadow before the first end-frozen cell (mirror of the start shadow)
export const frozenColumnShadowEndClassname = css`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's extract the common styles in frozenColumnShadowClassname and we can add frozenColumnShadowStart/EndClassname

Comment thread src/style/row.ts Outdated
import { css } from 'ecij';

import { cellFrozen } from './cell';
import { cellFrozen, cellFrozenEnd } from './cell';
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should rename it to be consistent

Suggested change
import { cellFrozen, cellFrozenEnd } from './cell';
import { cellFrozenStart, cellFrozenEnd } from './cell';

Comment thread src/style/row.ts Outdated
border-inline-start: var(--rdg-selection-width) solid var(--rdg-selection-color);
}

& > .${cellFrozenEnd}:last-child::after {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, let's dedupe

& > .${cellFrozen}:first-child::before,
& > .${cellFrozenEnd}:last-child::after { }

& > .${cellFrozen}:first-child::before { }

& > .${cellFrozenEnd}:last-child::after { }

@PaloSP PaloSP force-pushed the feat/end-frozen-columns branch from 5943b18 to 5016075 Compare April 24, 2026 20:57
@PaloSP
Copy link
Copy Markdown
Author

PaloSP commented Apr 24, 2026

@amanmahajan7 pushed a revision addressing all 5 comments.

On comment 4: intentional divergence from the literal suggestion. Comment 2's dedup removed the separate cellFrozenEnd export, so row.ts no longer imports from ./cell at all — its selectors use the DOM marker classes (.rdg-cell-frozen / .rdg-cell-frozen-end) directly. cellFrozen kept as the shared identifier per your phrasing in #2. Happy to rename cellFrozencellFrozenStart and restore an import if you prefer.

Two open questions, both breaking — your call:

  1. Rename CSS var --rdg-frozen-left-${idx}--rdg-frozen-start-${idx} for symmetry with new --rdg-frozen-end-${idx}?
  2. Rename DOM marker .rdg-cell-frozen.rdg-cell-frozen-start for symmetry with new .rdg-cell-frozen-end?

Both currently kept as-is (non-breaking). No rush — enjoy the vacation.

@PaloSP PaloSP force-pushed the feat/end-frozen-columns branch 4 times, most recently from c51325e to d88ce9d Compare May 6, 2026 18:14
@PaloSP
Copy link
Copy Markdown
Author

PaloSP commented May 13, 2026

Hi @amanmahajan7 — gentle nudge, no rush. Hope the vacation was good.

Branch is up-to-date with main (CI green) and the two open questions from my revision reply are still waiting on your call:

  1. Rename CSS var --rdg-frozen-left-${idx}--rdg-frozen-start-${idx} for symmetry?
  2. Rename DOM marker .rdg-cell-frozen.rdg-cell-frozen-start for symmetry?

Both are breaking, so I held off. Happy to address whenever you have a window.

Copy link
Copy Markdown
Collaborator

@amanmahajan7 amanmahajan7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more cleanup comments. I will do a detailed review soon.

Comment thread src/hooks/useCalculatedColumns.ts Outdated
const {
columns,
colSpanColumns,
lastFrozenColumnIndex,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
lastFrozenColumnIndex,
lastStartFrozenColumnIndex,

I think we can rename variables

}
if (frozenB) return 1;

// TODO: sort columns to keep them grouped if they have a parent
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can keep this TODO comment

Comment thread src/hooks/useCalculatedColumns.ts Outdated
}, [getColumnWidth, columns, lastFrozenColumnIndex]);
if (firstEndFrozenColumnIndex !== -1) {
const lastColumn = columns[columns.length - 1];
const lastMetric = columnMetrics.get(lastColumn)!;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const lastMetric = columnMetrics.get(lastColumn)!;
const lastColumnMetric = columnMetrics.get(lastColumn)!;

wdyt?

Comment thread src/hooks/useCalculatedColumns.ts Outdated
const lastColumn = columns[columns.length - 1];
const lastMetric = columnMetrics.get(lastColumn)!;
const gridEnd = lastMetric.left + lastMetric.width;
const firstEndMetric = columnMetrics.get(columns[firstEndFrozenColumnIndex])!;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const firstEndMetric = columnMetrics.get(columns[firstEndFrozenColumnIndex])!;
const firstEndFrozenColumnMetric = columnMetrics.get(columns[firstEndFrozenColumnIndex])!;

Comment thread src/style/row.ts Outdated
}

& > .${cellFrozen}:first-child::before {
& > .rdg-cell-frozen:first-child::before,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets rename this variable also

Suggested change
& > .rdg-cell-frozen:first-child::before,
& > .rdg-cell-frozen-start:first-child::before,

@PaloSP PaloSP force-pushed the feat/end-frozen-columns branch from 637e2f9 to 8c52472 Compare May 17, 2026 09:57
@PaloSP PaloSP force-pushed the feat/end-frozen-columns branch from 8c52472 to e9b24ff Compare May 17, 2026 10:03
@PaloSP
Copy link
Copy Markdown
Author

PaloSP commented May 17, 2026

@amanmahajan7 thanks — applied all 5 inline comments plus extended the rename principle consistently across the codebase.

Your 5 comments:

  1. lastFrozenColumnIndexlastStartFrozenColumnIndex
  2. Restored the // TODO: sort columns to keep them grouped if they have a parent comment ✓
  3. lastMetriclastColumnMetric
  4. firstEndMetricfirstEndFrozenColumnMetric
  5. .rdg-cell-frozen.rdg-cell-frozen-start (DOM marker, breaking) ✓

Same principle applied to similar names (where ambiguous "Frozen" meant "start-frozen"):

  • cellFrozencellFrozenStart
  • cellFrozenClassnamecellFrozenStartClassname
  • totalFrozenColumnWidthtotalStartFrozenColumnWidth
  • frozenColumnShadowTopClassnamefrozenColumnShadowStartTopClassname
  • columnMetric (inside the second useMemo) → lastStartFrozenColumnMetric
  • CSS var --rdg-frozen-left-${idx}--rdg-frozen-start-${idx} (breaking — this also resolves my open Q1 implicitly, since keeping the asymmetric -left after renaming the DOM marker to -start would be glaring)

All 351 tests pass on chromium + firefox.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants