Skip to content

feat(studio): sticky accordion group headers (stack top and bottom)#2128

Closed
vanceingalls wants to merge 1 commit into
graphite-base/2128from
studio-flat-09-sticky-group-headers
Closed

feat(studio): sticky accordion group headers (stack top and bottom)#2128
vanceingalls wants to merge 1 commit into
graphite-base/2128from
studio-flat-09-sticky-group-headers

Conversation

@vanceingalls

@vanceingalls vanceingalls commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

What

Ninth PR in the flat inspector stack (see #2120 for the foundation and full stack list). Group headers in the flat accordion — collapsed groups, the currently-open group's own header, and pinned-group headers — now stick to the top/bottom edge of the scroll container while scrolling, macOS Finder-style: headers above the current scroll position stack at the top, headers below stack at the bottom.

Stack: #2120 → ... → #2127#2128 (this).

Why

Requested after live manual testing — scrolling a long open group's content (Grade, with 14+ rows) scrolled its own header out of view along with everything else, losing the ability to see which group you're in or collapse/unpin it without scrolling back up.

How

Pure CSS, no new component logic: position: sticky with both top: 0 and bottom: 0 set on each header naturally sticks to whichever edge is being approached (top scrolling down, bottom scrolling up), and multiple sibling headers with the same styling stack against the edge in DOM order automatically — the same mechanism behind Finder/iOS Settings section headers.

  • Collapsed FlatGroup buttons, the open FlatGroup's header div, and PinnedGroupRow's header div all gained sticky top-0 bottom-0 z-10 bg-panel-bg.
  • The two nested headers (open-group and pinned-group, both sitting inside a padded outer wrapper) needed a -mx-4 px-4 bleed so their opaque background spans the full row width instead of stopping at the parent's padding edge — the collapsed-button case didn't need this since it's already a direct, full-width child of the scroll container.
  • Confirmed no intermediate ancestor between these headers and the outer scroll container has a competing overflow property that would break the sticky positioning.

Test plan

  • This is a pure-styling change with no new interactive behavior, so tests are simple className assertions confirming the sticky/background/z-index classes landed on the correct specific elements (the header div, not the whole group), plus confirming every pre-existing interactive test (pin/collapse/unpin clicks) is unchanged.
  • Important: neither the implementer nor I have browser access — the actual visual stacking/scroll behavior has NOT been visually verified and needs a live check in the running Studio dev server before considering this fully done. The CSS/DOM-nesting reasoning was independently re-verified in review as the best available substitute for a screenshot.
  • Full monorepo suite green (1582 tests, 0 failures); oxlint/oxfmt/fallow clean.
  • Unit tests added/updated
  • Manual testing performed — pending live visual verification, flagged above
  • Documentation updated (not applicable — internal Studio UI behind an off-by-default flag)

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed as part of the 15-PR edit-panel redesign stack. Full stack review posted on #2120. No blockers on this PR. — Miga

@miguel-heygen miguel-heygen left a comment

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.

Final current-head pass: 9232cc8. Reviewed the stack boundaries and current diff against the flat-inspector contracts; required checks have no failures/pending checks and no unresolved review threads remain. No new drift found; residual notes are non-blocking.

@vanceingalls
vanceingalls changed the base branch from studio-flat-08-style-grade-parity to graphite-base/2128 July 11, 2026 01:34

@miguel-heygen miguel-heygen left a comment

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.

Superseding earlier approval: the latest max-review pass found confirmed correctness issues in this flat-inspector stack (identity/selector-index state, hide-all write races, timing inference, slider pointer/keyboard/reset semantics, and duplicate React keys). Hold merge and require fixes plus re-review on the current stack head.

@miguel-heygen miguel-heygen left a comment

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.

Re-reviewed exact head 9232cc8fd1c0da7487827a18e6eb6f9a7d3d2594. This PR is not an ancestor of #2134: it remains a divergent sticky-header sibling while #2134 branches directly from #2127 and implements the later fixed-header/scroll-body design. Treating both as cumulative stack entries would land conflicting UX approaches.

Please close/supersede #2128, or explicitly restack the later branch on it and resolve the intended final behavior before merge. No independent code blocker beyond that stack-coherence/product decision was found in this delta.

Verdict: COMMENT
Reasoning: The layer is superseded/divergent rather than part of the advertised linear stack; its disposition must be made explicit.

— Deepwork

@miguel-heygen miguel-heygen left a comment

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.

Re-reviewed at unchanged head 9232cc8fd1c0da7487827a18e6eb6f9a7d3d2594.

Topology remains invalid for this stack: #2128 is based on graphite-base/2128, is not an ancestor of #2134, and is a divergent/superseded sibling of the actual #2127#2134 chain. There is no code or ancestry delta since the prior review.

Verdict: COMMENT (DO NOT MERGE; close or restack)
Reasoning: This PR is still outside the authoritative stack and would reintroduce a superseded branch of the header work.

— Deepwork

@miguel-heygen
miguel-heygen dismissed their stale review July 14, 2026 23:06

Stale stack-wide hold superseded by current topology review. #2128 remains a divergent/superseded sibling and should be closed rather than merged.

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.

3 participants