Repository navigation
White labels on status tags, sharper and aligned column headings - #56
Conversation
d18e3e5 to
d0663b5
Compare
Fivell
changed the title
d0663b5 to
95d70df
Compare
|
Before and after, zoomed. Top half of each image is 3.0.0 as released, bottom half is this branch. Same page, same data. Light
Dark
Three things change in the same frame:
The marker also still covers the whole cell for clicking: the link keeps |
95d70df to
c57e4cf
Compare
|
Rebased on #57, which this now sits on top of — the four commits collapse to three once that lands. #57 is right about something this branch had wrong, and it is my regression:
Top: 3.0.0 as released — the switch takes a line of its own and pushes the username, clock and logout onto a second row. Bottom: with Two changes to #57's workThe guard it added only fires while
The one real disagreement#57 makes the label white but leaves the fills as they were, which is 2.35:1 to 3.78:1 — its own comment says to set
Everything in one frameLight
Dark
Top half of each is 3.0.0 as released, bottom half is this branch. Tag labels go white on darker fills; headings line up with their columns instead of sitting 13px right of them; the sort marker moves beside the label and takes |
c57e4cf to
9891a41
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A duplicate Sass variable and incomplete declaration-to-documentation comparison should be corrected.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Improves status-tag contrast and index-table heading clarity and alignment.
Changes:
- Adds configurable, WCAG-compliant status-tag colors.
- Strengthens and realigns sortable table headings.
- Extends Sass override and README consistency checks.
| File | Description |
|---|---|
README.md |
Documents updated theme variables. |
active_admin_theme.scss |
Updates tag colors and table headings. |
test/css_check.rb |
Expands Sass and documentation checks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Both findings confirmed and fixed in 83be687. Duplicate declaration. Right, and it arrived when this branch rebased onto #57 — that PR added its own One-directional check. Also right, and the success line was the worse half of it: I added Verified by reintroducing each defect: The second one is how I noticed a |
04915c3 to
0d84f26
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The sort indicator misses accessibility contrast requirements, and README validation can overlook incomplete or duplicate entries.
Review effort: Balanced
Findings: 4
Open (5)
|
All five confirmed and fixed in 3c7823d. The numbers on the marker were exact — I measured the blend rather than trusting them: 0.6 it is. It is the only thing telling a sortable heading from a plain one, so 1.4.11 applies. Blank cells. Right, and worse than described: the name went into Duplicate rows. Also right, and symmetric to the declaration side I had already added. Reproduced by listing The contradictory comment. Fair — 2.35 to 3.78 belonged to the fills this palette replaces, not to the ones the sentence sits above. Reworded so the old range is identified as the old one. Screenshots reshot for the marker change. |
0d84f26 to
56fbbd9
Compare
56fbbd9 to
4c3f43b
Compare
4c3f43b to
aa8b126
Compare
|
Both confirmed, both fixed in aa8b126. The combined hole is real, and it is one I opened. Skipping blank cells was my fix for the previous finding, and it took blank rows out of duplicate detection along with it. Reproduced exactly as described — a blank row and a good row for the same name: Every row counts towards duplicate detection now, blank or not, and a blank cell also fails on its own rather than only being inferred from the undocumented check. The same case after: Four shapes checked, each reproduced before and after: blank + good row, blank row alone, blank dark twin, two good rows. The contract was overstated. |
|
This one is a false positive — the four images are in the change. GitHub's own files API for this PR lists them: The And the content differs where it should — 5.35% of Nothing to change here. Worth saying that this is the first of ten findings across five rounds on this branch that has not held up — the other nine were all real, including two in code I had written in answer to earlier ones. |
a0fb8fc to
62fb4d4
Compare
|
Confirmed in part, and the fix is bigger than the finding. 62fb4d4. I put the same low-contrast green behind the variable in every form Sass accepts and ran the guard on each: So Rather than widen the regular expression, the guard no longer parses colours at all. It appends a probe to the compiled source and lets Sass resolve them: .css-check-ok { r: red($skinStatusTagOkColor); g: green(...); b: blue(...); }Every form above now resolves, including the two that did not. The probe also raises if it comes back without all six colours, so the check cannot quietly stop covering anything — which is the failure mode that made this worth fixing rather than patching. Two things that came out of writing it:
|
|
Right, and the sharper version of the point is that my comment said the case was out of scope while the code let it through. Documented is not excluded. The probe reads Opaque colours are unaffected, in every form: I did not composite against the surfaces, as the alternative you offered. A tag sits on a table row, and the row is striped, hovered, selected and themed — there is no single backdrop to composite against, so any number it produced would be true for one of four states. Reporting is the honest answer, and the shipped palette has no reason to be translucent anyway. That makes three guards in this file that were quiet rather than wrong: the one-directional table comparison, the colour form it could not parse, and this. Same failure mode each time — the check appears to cover something it does not — which is worth more than any one of the defects. |
62fb4d4 to
c9abda0
Compare
aec5b37 to
513abcb
Compare
513abcb to
568572b
Compare
|
Another pass of my own over the branch as it stands. One finding worth fixing, two not. A blank row and a missing row gave the same message. Both reported Absent is measured against every row now, blank ones included, which also removed the set subtraction that existed only to stop the two messages overlapping. Also renamed Two things I checked and left alone:
|
| // 0.6, not lower: the marker is the only thing separating a sortable | ||
| // heading from a plain one, so WCAG 1.4.11 asks 3:1 of it. Against the | ||
| // header fill it gives 3.40 light and 4.22 dark; at 0.4 it was 2.13 | ||
| // and 2.73. | ||
| opacity: 0.6; |
| # `display` is written first in the stylesheet. Reorder the two lines in the | ||
| # source and the guard goes blind. | ||
| blocky = utility.select do |rule| | ||
| rule[/\{(.*)\}/m, 1].to_s.split(";").any? { |d| d.strip =~ /\Adisplay:\s*(?:flex|block|grid)\z/ } |
Two reports after 3.0.0, and what verifying them turned up. ## Status tags The label was black on a mid-tone fill. White on its own was the wrong fix — against the five shipped fills it lands between 2.35:1 and 3.78:1, under the 4.5:1 small text needs, which is why it went black during the review of activeadmin-plugins#49. So the fills move down with the label. Measured against white: neutral #8a909a -> #707681 3.21 -> 4.57 ok #8daa92 -> #5e7e63 2.53 -> 4.53 notice #6090db -> #3874d2 3.23 -> 4.57 warn #e29b20 -> #9e6c15 2.35 -> 4.56 error #d45f53 -> #ce483b 3.78 -> 4.55 The five fills join $skinStatusTagTextColor as variables, so the other choice is still one line away. They were literals inside the mixin call before. The label applies to filled tags; empty, unknown and none have no fill and keep $skinTextMutedColor, which the table now says. This pairing is the one thing here that has been got wrong twice — once by shipping white on a mid-tone fill, once by darkening the fill and leaving the label behind — and nothing was checking it. rake css now computes the ratio for every tag rule the theme emits and fails under 4.5:1. ## Column headings $skinTableHeaderTextColor was #5e6469, 4.93:1 against the header fill while the rows underneath run at 10.15:1 — passing, but visibly softer than the data it labels, which is what made it look blurry. It takes $skinTextColor now. They also did not line up with their own columns. ActiveAdmin puts the sort arrow inside the heading link, on the left, cleared with `padding-left: 13px`. Cell and heading share the same 12px padding, so the cell is aligned, but the label inside is pushed 13px right while the data below starts at the padding edge. Measured on a block column of status tags, the shape the report came from: cell left 559.9 559.9 heading link left 571.9 571.9 tag left 571.9 571.9 heading TEXT left 584.9 571.9 Moving the image to the right edge is not enough: the link is `display: block`, so the arrow would park at the far side of the column. It is a pseudo-element now, which puts it beside the label, keeps the link full width so the whole cell stays clickable, and lets the marker take currentColor — the stock sprite is a fixed grey PNG that cannot follow the text into dark mode. At 0.6 opacity it is 3.40:1 light and 4.22:1 dark against the header fill; it is the only thing separating a sortable heading from a plain one, so WCAG 1.4.11 asks 3:1. The unused side of the triangle has no width rather than a transparent one, so the box is as tall as the mark in it and both sort states sit at the same height. Keeping all four sides and nudging the ascending one with a margin put them visibly apart, because `vertical-align: middle` centres the box and the visible half then sits off-centre within it. ## The check that was supposed to prevent this rake css compares the README variables table against the declarations. It was doing so in one direction only, and loosely: - a variable with no row in the table passed, while the success line claimed the table matched every declaration; - a blank cell counted as documentation — the name entered the documented set, which exempted it from the undocumented check, while the mismatch check skipped it for having no value, so it passed on both sides; - skipping blank cells outright then opened a narrower version of the same hole: one blank row and one good row for the same name, where the good row satisfied the lookup and the blank one never reached duplicate detection; - a name listed twice kept the last row, so the table could agree with the stylesheet while a reader meets the stale row first; - a name declared twice went unnoticed, though Sass keeps the first !default and drops the rest. Every row counts towards duplicate detection now, blank or not, and a blank cell fails on its own. Each hole was reproduced before and after. The advertised count comes from the declarations rather than a hand-maintained constant that said 52 while 81 variables were being compared.
568572b to
6128a73
Compare
Minor, not major: the variable contract only grows. Six status tag colours join it, every default that moved is reachable through one of them, and nothing that worked against 3.0.0 stops working. What moved since 3.0.0: #57 the theme switch no longer breaks the utility nav onto two lines #56 status tag labels are white on darker fills, clearing 4.5:1 where the old pairing ran 2.35 to 3.78; column headings take the body text colour and line up with their own columns; the sort marker is drawn by the theme in currentColor instead of ActiveAdmin's grey sprite Upgrading gains a 3.1.0 section for the two colour changes, and the 3.0.0 one is corrected — it said two things had changed and then listed three. Both artefacts checked: the gem carries the stylesheet and the switch script, and npm run prepublishOnly + npm pack puts src/active_admin_theme.scss and src/theme_toggle.js in the tarball. The npm install snippet moves to ^3.1.0.
Two reports after 3.0.0 — the label inside a status tag should be white, and index-table column headings read too soft — plus what verifying them turned up.
Status tags
White on its own was the wrong fix. Against the five shipped fills it lands between 2.35:1 and 3.78:1, under the 4.5:1 small text needs, which is exactly why the label went black during the review of #49. So the fills move down with it:
#8a909a→#707681#8daa92→#5e7e63#6090db→#3874d2#e29b20→#9e6c15#d45f53→#ce483bThe five fills join
$skinStatusTagTextColoras variables, so black-on-pale is still one line away. They were literals inside the mixin call before.Column headings
Too soft.
$skinTableHeaderTextColorwas#5e6469, 4.93:1 against the header fill while the rows underneath run at 10.15:1. Passing, but visibly weaker than the data it labels. It takes$skinTextColornow.Not above their own columns. ActiveAdmin puts the sort arrow inside the heading link, on the left, cleared with
padding-left: 13px. The cell and the heading share the same 12px padding, so the cell is aligned — but the label inside is pushed 13px right while the data below starts at the padding edge. Every sortable column is affected, not only the one reported.Moving the background image to the right edge is not enough: the link is
display: block, so the arrow would park at the far side of the column instead of beside the label. It is a pseudo-element now, whichcurrentColor— the stock sprite is a fixed grey PNG that cannot follow the text into dark mode.At
0.6opacity the unsorted marker is 3.40:1 light and 4.22:1 dark against the header fill. It is the only thing separating a sortable heading from a plain one, so WCAG 1.4.11 asks 3:1 of it.The check that should have caught this
rake csscompares the README variables table against the declarations. It was doing so in one direction, and loosely. Four holes, each reproduced before and after the fix:!defaultwith no row, while the success line claimed the table matched every declaration!defaultand drops the restThe advertised count comes from the declarations now, rather than a hand-maintained constant that said 52 while 81 variables were being compared.
Screenshots
All four README sheets are reshot. The dummy index carries a
Tagscolumn under a sortable heading — the block-column shape the reports came from — labelled release / draft / featured / deprecated / review, which between them use all five tag colours; the previous pair showed two.Before and after, zoomed, in the comments: light and dark.
Supersedes #55.