Repository navigation
feat(row-model)!: group expansion defaults to expanded - #350
Merged
Merged
Conversation
Restores the default #321 changed by accident. grid-core shipped `groupsDefaultExpanded ?? true` for the whole life of the grouping feature; the incremental row model's `createExpansionRoot` fell back to `{ kind: "collapsed" }`, and the rename from a boolean to a policy object carried the flip along with it unremarked. Expanded is also the right default here, not merely the previous one. Grouping in pretable is an interactive act as much as a configuration one — the user drags a column into the group panel while reading their rows — and collapsing on drop hides the data they were just looking at. AG Grid (`groupDefaultExpanded: 0`, verified in its shipped defaults) and TanStack (empty expanded map, verified in source) both default collapsed, but in those grouping is typically developer configuration applied before the user arrives. `{ kind: "through-depth" }` remains the escape hatch for populations too large to draw at once. The docs described all three policies and never said which you get by omission. That silence is why a shipped behaviour change went unnoticed; grouping.mdx now states it. Tests that depended on starting collapsed now say so explicitly rather than inheriting whichever default ships — which is the same class of bug this commit fixes. aria-rowcount under grouping moves 3 -> 5 (two group rows, two data rows, one header); the rule it pins is unchanged, in that the external total is still not published while grouping is active.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
blove
enabled auto-merge (squash)
August 13, 2026 04:35
Contributor
Vercel preview readyPreview: https://pretable-9a35txe15-cacheplane.vercel.app Updated automatically by the |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Groups now start expanded. Closes the open decision on #327.
What this restores
grid-coreshippedgroupsDefaultExpanded ?? truefor the entire life of the grouping feature. The incremental row model'screateExpansionRootfell back to{ kind: "collapsed" }instead, and because the option was renamed from a boolean to a policy object at the same time, the flip rode along unremarked. Nobody chose collapsed on the merits.Why expanded is right, not just previous
Grouping in pretable is an interactive act as much as a configuration one — the user drags a column into the group panel while reading their rows. Collapsing on drop makes the data they were just looking at disappear at the moment they acted.
The comparison, verified in the installed packages rather than from memory:
groupDefaultExpanded: 0→ collapsedgridOptionsDefaultand runtime bundlegetDefaultExpandedState()→ empty map → collapsedrowExpandingFeature.utilsSo this deliberately diverges from both comparators I could check. The reason is that in those, grouping is typically developer configuration applied before the user arrives, where collapsed is a sensible landing state.
{ kind: "through-depth", depth }remains the escape hatch for populations too large to open at once.The documentation gap that hid it
grouping.mdxdescribed all three policies and never said which you get by omission. That silence is why a shipped behaviour change went unnoticed for a day. It now leads with the default.Test changes, and why each is not a papering-over
Six tests failed. None of them were asserting the default on purpose except one:
expansion.test.ts > defaults collapsed…— renamed and inverted. This is the test that pinned the old behaviour; it now pins the new one, and additionally asserts an explicitly-collapsed model still yields 2 visible rows.setGroupExpanded(x, true)expecting an override to be created. Under an expanded default that is a no-op, which is correct. They now pass{ kind: "collapsed" }explicitly, so they keep testing override storage relative to a default rather than inheriting whichever one ships — the same class of bug this PR fixes.grouping.test.ts > …collision-proofreads groups out ofrange(0, 20). 11 groups over 12 rows is 23 visible once expanded, so a 20-row window silently truncated to 10. Widened torange(0, 100); the test is about group identity and has nothing to do with expansion.server-authority-aria.test.tsx > downgrades to the loaded model countmovesaria-rowcount3 → 5: two group rows, two data rows, one header. The rule it pins is unchanged — the external total of 5432 is still not published while grouping is active.Not in scope
The bench needs no change: its
groupplan predictsrows + groupCount, which is correct again under an expanded default, so the 2026-08-10 grouping milestone stays comparable rather than needing a re-measure. I did not pininitialExpansionin the bench even though pinning is good practice, because #327 means grouping does not currently apply there at all and I could not verify the change end to end.Verification
row-model311,react828,website368,core7,bench128 — all pass.pnpm build && pnpm api:checkclean, so the public surface is unchanged; this is a behavioural change to a documented default, carried by a minor changeset.🤖 Generated with Claude Code