Repository navigation
Keep the admin chrome in view while tables scroll - #71
Merged
Merged
Conversation
Fivell
force-pushed
the
feat/sticky-action-buttons
branch
from
October 14, 2026 11:12
201b33b to
8a042e7
Compare
Fivell
changed the title
Fivell
force-pushed
the
feat/sticky-action-buttons
branch
2 times, most recently
from
October 14, 2026 12:05
d884e11 to
ec1faba
Compare
On a wide index table the whole page scrolls horizontally. Pin #header and #title_bar to the viewport's left edge and cap them to the viewport width, so the logo, nav, page title AND action buttons all stay visible while the table scrolls underneath (no `top`, so they still scroll vertically). #titlebar_right resets to static — the bar does the pinning now, not the buttons. Width is min(100vw - var(--scrollbar-width), 100%): plain 100vw includes the vertical scrollbar gutter and shifts the left-pinned bars at the far-right scroll position. The optional wigu/scrollbar_width.js sets --scrollbar-width; without it the var falls back to 0 (plain 100vw). Consuming apps each solved this with their own CSS (scroll_buttons_init.scss, title_bar overrides) and JS — this owns it for all of them.
Fivell
force-pushed
the
feat/sticky-action-buttons
branch
from
October 14, 2026 12:10
ec1faba to
955a23a
Compare
The viewport-width cap on #header and #title_bar was written as
unquote("min(calc(100vw - var(--scrollbar-width, 0px)), 100%)"). dart-sass
emits that fine, but sprockets consumers (RouteServer, pbx) compile the theme
through sassc/libsass, whose Sass min() function tries to evaluate its
arguments numerically and rejects calc() with "is not a number for `min'" —
failing the whole asset build.
Express the same cap with a plain CSS max-width: 100% and keep the width as a
bare calc(), which libsass passes through untouched. Behaviour is identical in
every browser; the npm (dart-sass) consumers are unaffected since src/ is
generated from this file by the prepare script.
Keep an index table's column headers visible while scrolling it vertically, replacing the per-app implementations (RouteServer shipped a 280-line jQuery stickyTableHeaders plugin keyed off hardcoded table ids; api-3 had its own CSS). Scoped to .paginated_collection so it only applies to real ActiveAdmin index listings, not custom report/dashboard tables. A listing opts out with .skip-sticky-headers; a non-index table_for opts in with .with-sticky-headers. top: -1px hides the 1px header border seam. The header and title bar pin only horizontally (no top), so they scroll away vertically and the thead pins to the viewport top with no overlap.
#header and #title_bar carried the same sticky + 100vw-minus-scrollbar + max-width pin, comment and all, twice. Pull it into a @mixin viewport-pinned-bar so the width mechanics and their rationale live in one place; @include it from both. The mixin inlines, so the compiled CSS is unchanged. Also drops a stray empty declaration (double semicolon) in the breadcrumb colour.
There was a problem hiding this comment.
🟡 Changes recommended
Stacking-order and Turbo navigation issues can obscure menus or leave the viewport-width calculation stale.
4 open findings
What changed in this PR
Pins ActiveAdmin’s header and title bar during horizontal scrolling and adds supporting scrollbar-width detection.
Changes:
- Adds horizontally sticky header/title-bar styling.
- Adds optional scrollbar-width JavaScript and demo page.
- Enables vertical sticky headers for index tables.
| File | Description |
|---|---|
active_admin_theme.scss |
Adds sticky chrome, flex title layout, and sticky table headers. |
scrollbar_width.js |
Publishes scrollbar width as a CSS property. |
active_admin.js |
Loads the new script in the dummy app. |
wide_table.rb |
Adds a wide-table demonstration page. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+15
to
+21
| window.addEventListener("resize", update); | ||
| document.addEventListener("DOMContentLoaded", function () { | ||
| update(); | ||
| // The presence of a vertical scrollbar can change after load (content grows/ | ||
| // shrinks) without firing resize — watch the body's box too. | ||
| if (window.ResizeObserver) { new ResizeObserver(update).observe(document.body); } | ||
| }); |
Comment on lines
+675
to
+679
| position: sticky; | ||
| left: 0; | ||
| width: calc(100vw - var(--scrollbar-width, 0px)); | ||
| max-width: 100%; | ||
| box-sizing: border-box; |
Comment on lines
+1355
to
+1361
| // Sticky table headers: keep the column labels visible while a long index table | ||
| // scrolls vertically. The header/title bar pin only horizontally (no `top`), so | ||
| // they scroll away on vertical scroll and this thead pins to the viewport top — | ||
| // no overlap. Scoped to .paginated_collection so it only hits real AA index | ||
| // listings, not custom report/dashboard tables. Opt a listing out with | ||
| // .skip-sticky-headers; opt a non-index table_for in with .with-sticky-headers. | ||
| .paginated_collection table.index_table:not(.skip-sticky-headers) thead tr th, |
Comment on lines
+8
to
+9
| // them to the visible viewport instead, so they stay put. Optional: the CSS falls | ||
| // back to `var(--scrollbar-width, 0px)` = plain 100vw when this script is absent. |
… theme So consumer apps don't each re-style the title bar: - #title_bar h2: font-size 16px + margin:0 + nowrap. AA's default (~31px) towered over the breadcrumb and every app shrank it locally (RS had lost its override and showed the huge default). - #titlebar_right: 10px gap between multiple action items (was flush). - delete links: also match a.delete-link (hyphen), not just a.delete_link. Lets RouteServer/api-3 drop their local title_bar/titlebar overrides entirely.
Fixed height: 37px on the flex bar plus an unconditional nowrap title broke containment on both axes: a long record title fixed #titlebar_left's min-content width, pushed the action buttons past the bar's right edge (unreachable on wide pages, where the sticky bar follows horizontal scroll), and action items wrapping on a narrow window overflowed the 37px bar over the content below. min-height instead of height, min-width: 0 down the title chain, and nowrap + ellipsis on the h2 keep the compact row without losing the buttons.
right/top: auto are inert on the position: static box they accompany (offsets apply to positioned elements only), and float: none countered a float that neither AA base nor any consumer path applies — float does not apply to flex items, and span.breadcrumb is a direct flex child. All three delete with no rendering change; as written they sent the next reader hunting for base rules that don't exist. margin-bottom: 0 stays — base sets 12px, that one is live.
The 37px minimum was a hardcoded literal in a theme whose public surface is the README-paired $skin*!default variables — including the very button paddings that size the bar's content: a consumer raising $skinTitleBarButtonPaddingY past the band had no variable to rebalance with. $skinTitleBarHeight: 37px!default follows the $skinTableToolsHeight precedent; css_check's README parity guard covers the new row.
ActiveAdmin registers the stylesheet with media "all" and the theme has no print scoping, so the title's nowrap leaked into print and cut titles longer than ~85 characters at the page box's right edge — paper has no horizontal scroll, and master wrapped them. Scope the nowrap/ellipsis trio to @media screen; the flex bar itself is harmless in print once the title can wrap.
The flashes bar sits between the two newly pinned bars in the same horizontally scrolling wrapper, so on a wide page scrolled right the user saw a colored band whose text had scrolled off-screen. Apply the same viewport-pinned-bar mixin; AA keeps all padding/background on the inner .flash, so the always-rendered empty wrapper stays inert on flash-less pages.
The comment told consumers to opt a listing out with "index class: skip-sticky-headers", but AA 3.x hardcodes the index table class attribute and silently ignores that option — the documented escape hatch did not exist. Name the real ones: a one-line app-wide CSS override for AA listings, .skip-sticky-headers only for self-rendered tables, and .with-sticky-headers to opt hand-built tables in. (README section follows in a separate commit.)
The corner th carry inner-radius top rounding to nest inside the table's rounded border, but the rounded corners are transparent: once the sticky header detaches on scroll, the rows beneath flicker through the ~3px notches at both ends of the pinned strip. Square the corners for sticky-enabled tables only — the static at-rest artifact (square background over the border rounding) is the lesser of the two, and CSS cannot distinguish stuck from at-rest.
Every thead row's th stuck at the same top: -1px, so a table with a two-row thead (group header over column header) had its header rows overlap while stuck — the standard multi-row-sticky failure that needs per-row offsets CSS alone can't express. table_for emits a single header row, but .with-sticky-headers is public API for hand-built tables once released: scope to tr:first-child so the first row pins and further header rows scroll away normally instead of painting over it.
The ResizeObserver was created inside a DOMContentLoaded listener and watched document.body: loaded deferred/async it never attached (DCL already fired), and under Turbo/Turbolinks it kept observing the detached old body after the first navigation — leaving --scrollbar-width stale in both directions. Observe document.documentElement instead: it exists at parse time, survives body swaps, and its client box resizes on the same content growth that toggles the scrollbar. The DCL gate goes away entirely.
update() wrote the custom property on every event — resize and the ResizeObserver both fire per frame during a drag-resize, and every content growth pings the observer — for a value that only changes when the scrollbar toggles. Same-value dedupe for inline custom properties is engine-dependent and the var feeds the pinned bars' width calc, so guard with a last-value check before setProperty.
The bare name is the only non --aa-* custom property in the theme and collides two ways with consumer tokens of the same popular name. It is deliberate: legacy consumer scripts set the exact same variable with the same semantics, so the theme works there before the manifest is updated, and renaming to --aa-scrollbar-width would make the require mandatory at upgrade. State the trade-off at the definition site.
WIDE_COLS/WIDE_ROWS were assigned inside the register_page block, which is lexically top-level — they landed on Object, and AA's reloader re-loads every app/admin file on dev reload, re-assigning them with an already-initialized-constant warning each time. Locals inside the content block avoid both the warnings and the namespace leak.
The th interpolation went into the html_safe builder unescaped while the td two lines below is ERB-escaped. The values are static today, but dummy demos are what consumers copy — parameterizing the headers along the existing line would ship unescaped markup into html_safe output. Escape uniformly, matching the sibling table.rb.
The feature shipped matched by nothing: the dummy has no database-backed resources (so no AA index table ever renders inside .paginated_collection), the hand-built demo table lacked the opt-in class, and no test visited the demo page — a selector typo or an AA DOM rename would have passed every suite silently. Opt the wide-demo table in via .with-sticky-headers, give it enough rows to scroll, and assert the computed position is sticky.
The new opt-in script and both public classes were documented nowhere — every other shipped wigu script has its Sprockets require and npm import in the README. Without the require, consumers keep the ~15px far-right bar shift the PR exists to fix, and the fix was undiscoverable; the sticky headers section also names the working app-wide opt-out (AA 3.x ignores index class:, so the class alone cannot reach real index tables).
Public repo comments should not reference consumers' internal file names; 'local stylesheet overrides' says the same thing.
Fivell
changed the title
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.
What
The theme keeps the admin chrome usable while tables scroll, in both directions:
Pinned bars (horizontal scroll)
#header,#title_barand the flash bar are pinned to the viewport's left edge and capped to the viewport width via a sharedviewport-pinned-barmixin, so the logo, nav, page title, action buttons and flash messages all stay visible while the table scrolls underneath (notop, so they still scroll vertically).#titlebar_rightresets tostatic— the bar does the pinning now, not the buttons.calc(100vw - var(--scrollbar-width, 0px))capped bymax-width: 100%(sassc evaluates Sass's ownmin()and rejectscalc()inside it, hence the two-property form). Plain100vwincludes the vertical-scrollbar gutter; the optionalwigu/scrollbar_width.jssets--scrollbar-width, and without it the var falls back to0— graceful degrade, just a ~15px far-right shift. The script observesdocument.documentElement, so it works loaded deferred and survives Turbo/Turbolinks body swaps, and it skips same-value writes.Sticky table headers (vertical scroll)
top, so the two never overlap. Only the first thead row pins (multi-row theads would overlap at one shared offset), and the corner cells square off so rows don't flicker through the rounded corners' transparent notches while stuck.index class:is ignored), so the README documents the app-wide one-line CSS opt-out;skip-sticky-headers/with-sticky-headerscover self-rendered tables.Title bar
$skinTitleBarHeight(new$skin*variable, README-paired) and growing to fit: long record titles ellipsize on screen instead of pushing the buttons out of the pinned bar, wrap normally in print, and wrapping action items grow the bar instead of overflowing it.Why
Consumer apps each kept their own copy of this chrome: a scrollbar-width script,
positionoverrides pinning the action buttons, and title-bar restyling. Centralizing it in the theme lets them delete all of that.Hardening
The branch went through a max-depth agent review plus Copilot; every confirmed finding is fixed: title-bar containment on both axes, print behavior, the flash-bar gap, sticky-corner artifacts, multi-row thead scope, observer lifetime under Turbo/deferred loading, write dedupe, truthful opt-out docs, demo-page constants/escaping, and test coverage that pins the sticky selector to a real element (a selector typo now fails the suite).
Testing
rake css— the theme compiles against AA and the stylesheet guards hold (incl. README parity for the new$skinTitleBarHeight).rake test— the Capybara dummy-app suite asserts the wide-demo table's computedposition: stickyalongside the existing clock/logout contract.Preview
Before — the title bar scrolls away with the table
After — title and action button stay pinned