fix(tabs): mark the active tab in the brand colour, not the rule colour - #98
Conversation
The selected indicator was `selected:border-b-2 selected:border-color-border`, and `border-color-border` is the same token the `list` slot draws its own rule with. A selected tab therefore marked itself with a thicker length of the very line it was sitting on: on a phone it read as a grey smudge under one label rather than a selection. `selected:border-primary` resolves through the consumer's brand colour, so the indicator is the accent every other selected surface in an app already uses. The test asserts the two rendered border COLOURS differ rather than the className, because the className was never invalid: it resolved to the background it was drawn over. Verified non-vacuous by restoring the old token, which collapses the set to a single colour.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. The fix is right and the diagnosis holds - the indicator and the rule really did share Minor
TestsThe new case covers the regression and the author's non-vacuity check (restoring the old token collapses the set to one colour) matches what the code does; it under-specifies the positive half, as above. No dark-mode case. Checks I ran
|
|
@kodizm review |
1 similar comment
|
@kodizm review |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Re-review: the branch still carries the single commit I reviewed - the same one-line token swap with its comment, and the same test - so nothing new to raise, and my earlier notes stand as they were. Minor
Tests
Checks I ran
|
The review is right on all three points and I verified each before changing anything. `border-primary` is a bare palette utility, not a semantic role. There is no `border-color-primary` key in the alias map (only `border-color-border` and `border-color-border-subtle`), so the alias layer passes the token through untouched, and wind's border parser defaults a missing shade to 500 (`border_parser.dart:203`). Meanwhile `bg-primary` resolves to `bg-primary-600 dark:bg-primary-500`. So an active tab underlined in primary-500 next to a primary-600 button: one shade off, and with no `dark:` half at all, against this project's own rule that light and dark are always paired. Now `selected:border-primary-600 dark:selected:border-primary-500`, matching the `dark:hover:` prefix order already used in five places here. The test asserted "more than one distinct bottom-border colour", which pins the indicator away from the rule and to nothing: `border-red-500` kept it green. It now asserts the indicator IS the brand shade, in light and in dark, and both assertions fail on the bare token. Two things measured rather than guessed while writing them. The colour comes from `WindThemeData().colors['primary']` rather than a hex, because the first attempt used Material's `Colors.blue.shade600` (#1E88E5) while wind's default primary is Tailwind blue (#2563EB). And the dark case needs `WindThemeData(brightness: dark, syncWithSystem: false)`: a `MediaQuery` platform-brightness override does not reach wind, which reads its own theme data, so the first version of that case asserted light twice and looked fine. Plus the `[Unreleased]` entry the post-change checklist asks for.
|
All three verified and fixed. The bare token. Confirmed exactly as described: no The under-specified assertion. Right, and it was the more useful of the two points: Two things I had to measure rather than reason, both of which would have shipped a test that looked fine and proved nothing:
The changelog. Added under Gates on the tip: |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental re-review of What changed since Two things I checked rather than assumed:
Tests
Checks I ran
|
What
MSTabs's selected indicator moves fromselected:border-color-bordertoselected:border-primary.Why
border-color-borderis the same token thelistslot draws its own rule with, so a selected tab marked itself with a thicker length of the very line it was sitting on. Reported from a phone, where it reads as a grey smudge under one label rather than a selection.Testing
test/ui/components/tabs/tabs_test.dartasserts the rendered bottom-border colours of the indicator and the rule DIFFER. Asserted on colour rather than className because the className was never invalid: it resolved to the background it was drawn over. Verified non-vacuous by restoring the old token, which collapses the set to one colour.