Compile the theme in CI and reject wrong-typed variables - #50
Merged
Merged
Conversation
The gem ships SCSS, not CSS, so nothing in this repo's build ever compiled the theme file. A change that only breaks under a particular variable override reached consumers with nothing to catch it — there was no CI at all. Two parts: * `rake css` (test/css_check.rb) compiles the theme together with ActiveAdmin's own stylesheets in five override configurations, and asserts three wrong-typed ones are rejected. Wired into a GitHub Actions workflow on Ruby 3.1 and 3.4. * Type guards on the five public variables. A wrong-typed override is legal SassScript, so it compiled without a word and emitted CSS the browser drops (`border-bottom: none solid #5ea3d3`, `padding: 10 12px`): the rule silently vanished and ActiveAdmin's own value reappeared. It now fails the build with the variable name and the value it got. No change to the generated CSS for any valid configuration.
Fivell
force-pushed
the
chore/compile-check
branch
from
October 1, 2026 09:24
f778d63 to
9732c23
Compare
Fivell
added a commit
that referenced
this pull request
Oct 2, 2026
* add menu colors variables * more variables * Fix the defects found reviewing the header menu variables Review of the two commits above surfaced 13 defects. The evidence for each is in docs/pr-47-fixes/ — one folder per defect, with before/after screenshots taken by compiling the theme against ActiveAdmin's own stylesheets and driving the menu with a real cursor in Chromium. Ships to every project, configured or not: * `width: auto; max-width: none` plus `white-space: nowrap` removed every bound on dropdown width, so a long label pushed the panel out of the viewport. Scrolling to reach it breaks the :hover chain that holds the menu open, so the item was unreachable by mouse. Measured at a 1280px viewport: 132.5px off-screen, scrollWidth 1420. Now content-sized with a ceiling ($skinMenuPanelMaxWidth) and wrapping past it. * Narrowing the bridge border to `> a` also stripped it from every dropdown anchor, where it had been acting as 7px of row separation. Rows went 34.3px to 27.3px. Restored as padding instead of a side effect. * $skinHeaderPaddingY collapsed the header's asymmetric 5px/9px into 7px each. Same total height, but every header element moved 2px down. Split into $skinHeaderPaddingTop/Bottom keeping the original values. Only reachable through the new variables: * A hard-coded `color: #ffffff` out-specified $skinMenuTextColor, so the variable had no effect on the hovered or the current item — and `.current` is not transient, so the current page's entry was invisible permanently. * The top-level pill was themable but its text was not; added $skinMenuPillTextColor. * $skinMenuFontSize sat on the `li`, so it rescaled the whole dropdown subtree rather than the header menu its comment describes. * The marker arrow kept a magic `top: 12px` while item height became configurable, and was pinned to a fixed-palette PNG that does not follow $skinMenuPanelColor. Now centred, and drawn in currentColor so it tracks the text colour. * The pill/panel junction showed the header colour through four rounded corners, and the bridge was 7px against ActiveAdmin's 5px gap. * `li.has_nested a` gave every leaf item a 20px arrow gutter it does not need. * `:focus` was unstyled, so keyboard navigation had no indicator. * $skinTitleBarButtonPadding* reached one of the two buttons in the title bar; the batch-actions button beside it stayed on the literal. * Wrong-typed overrides now hit the type guards added in #50, which this commit extends to all thirteen new variables. `rake css` covers the first and last of these directly: 8 override configurations compile, 6 wrong-typed ones are rejected, and no hard-coded white is left in the header menu rules. The same check reports 4 problems against the two commits below. Known limit, commented in place: a depth-2 flyout under a right-edge tab can still leave a 1280px viewport, down from 132.5px to 69.5px. CSS cannot see the distance to the viewport edge, and ActiveAdmin's own 175px panels already land within 5px of it. * Default the pill text colour to the dropdown text colour $skinMenuPillTextColor defaulted to #ffffff independently of $skinMenuTextColor, so a project that light-themes the menu had to discover and set two variables instead of one: setting the panel and the dropdown text left the top-level pill label white on a light fill. Found by shooting the menu against a real ActiveAdmin admin rather than by reading — it is the same mistake this branch fixes elsewhere, made one variable over. * Drop the dead title-bar batch-actions block instead of varying it The previous commit claimed to fix "only one of eight identical `padding: 10px 20px` rules was extracted" by applying the new variables to `#title_bar #titlebar_right div.batch_actions_selector` as well. That rule matches nothing: ActiveAdmin renders the batch-actions selector only inside `div.table_tools` — `build_table_tools` in lib/active_admin/views/pages/index.rb, unchanged across 3.0 through 3.5 — so the title bar never contains one. The claim came from a hand-written screenshot harness whose markup I had invented, with a batch-actions button sitting inside `#titlebar_right`. Shooting the same scenario against a generated ActiveAdmin admin showed the real markup and the mistake with it. So there is no second button beside the title-bar action button to disagree with, `$skinTitleBarButtonPadding*` on `.action_item a` is complete as it stands, and the block this removes has been dead CSS for the whole supported ActiveAdmin range. The two live copies under `div.table_tools` keep their literal padding: they are a different row, and giving them a variable named for the title bar would be worse than leaving them alone. * Drop the screenshot evidence folder It was a point-in-time record of reviewing these commits, and it does not belong in the gem: the before/after pairs are reproducible on demand against a generated ActiveAdmin admin, so carrying ~450 KB of PNGs and a static harness in the repository buys nothing that regenerating them does not. One of its cases was actively wrong. 09-titlebar-button-padding was shot against markup I had hand-written, with a batch-actions button inside #titlebar_right — ActiveAdmin puts it in div.table_tools and never there. That invented markup is where the withdrawn finding came from. --------- Co-authored-by: Dmytro <dmitry.s@yeti-switch.org>
Fivell
added a commit
to yeti-switch/active_admin_theme
that referenced
this pull request
Oct 2, 2026
master has moved three commits ahead: the gem file whitelist (activeadmin-plugins#48), the compile check and variable type guards (activeadmin-plugins#50), and the header menu work with its review fixes (activeadmin-plugins#51). activeadmin-plugins#51 matters here. Its first two commits are this branch's first two, cherry-picked, with thirteen defects fixed on top — and it was squash-merged, so git cannot tell they are already upstream. A rebase would have replayed them and quietly reverted every one of those fixes; merging keeps them. The stylesheet conflicted in eight places. Resolution: * Variable header: both sides kept. This branch's palette and the per-mode *Dark values stay; the eight menu variables the two sides share take master's definitions, because master's are the corrected ones — $skinMenuPillTextColor and $skinMenuItemHoverTextColor now default to $skinMenuTextColor instead of #ffffff independently, $skinMenuItemPaddingY is 8px, $skinHeaderPaddingY is split back into Top/Bottom so the header does not shift, and $skinMenuPanelMaxWidth comes across. The type guards from activeadmin-plugins#50 follow the variables. * Menu rules: master's throughout — the width ceiling, the squared pill and panel corners, the 5px bridge, the currentColor marker, :focus. * #utility_nav and ul.tabs > li font-size: this branch's, master has nothing there. * The dead #title_bar batch-actions block stays deleted. Verified after the merge: the stylesheet compiles, and every fix from activeadmin-plugins#51 is still present in it.
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
1 participant
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.
The gem ships SCSS, not CSS, so nothing in this repo's build ever compiles the theme file — and there is no CI at all. A change that only breaks under a particular variable override reaches consumers with nothing to catch it.
This adds the missing gate, and nothing else. No change to the generated CSS for any valid configuration.
rake csstest/css_check.rbcompiles the theme together with ActiveAdmin's own stylesheets (it is already a runtime dependency, so no new one is needed) in five override configurations, and asserts three wrong-typed ones are rejected:Wired into a GitHub Actions workflow on Ruby 3.1 (the gemspec minimum) and 3.4.
Type guards on the five public variables
A wrong-typed override is legal SassScript, so today it compiles without a word and emits a declaration the browser drops — the rule silently vanishes and ActiveAdmin's own value reappears, with nothing in the build output to say why:
$skinTablePadding: nonepadding: none 12px none 12px$skinBorderRadius: 4(no unit)border-radius: 4Now:
Why now
Both open theme PRs add variables of these kinds — #47 adds 13, #49 adds 65 — and neither can be checked by anything today. Landing the gate first means both inherit it instead of each carrying its own.
One concrete case the check already covers:
$skinBorderRadius: 0.25remcompiles cleanly on master (it is in theGOODlist) and aborts the whole asset pipeline on #49's head withIncompatible units: 'px' and 'rem', because that branch introduces$skinBorderRadius - 1px. CI would have caught it on the PR.