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
| | `$skinTableHeaderTextColor` / `$skinTableHeaderTextColorDark` | `#5e6469` / `#dde2e8` | index-table column header text | | ||
| | `$skinStatusTagTextColor` | `#ffffff` | status tag label; `#000000` passes WCAG AA on every fill | | ||
| | `$skinTableHeaderTextColor` / `$skinTableHeaderTextColorDark` | `$skinTextColor` / `#dde2e8` | index-table column header text; the body text colour, so headings read as strongly as the rows | | ||
| | `$skinStatusTagTextColor` | `#ffffff` | label inside every status tag | |
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. ## 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 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; - 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. All four fail now, each 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.
56fbbd9 to
4c3f43b
Compare
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.