Skip to content

fix(tabs): mark the active tab in the brand colour, not the rule colour - #98

Merged
anilcancakir merged 2 commits into
mainfrom
fix/tabs-active-indicator
Aug 21, 2026
Merged

fix(tabs): mark the active tab in the brand colour, not the rule colour#98
anilcancakir merged 2 commits into
mainfrom
fix/tabs-active-indicator

Conversation

@anilcancakir

Copy link
Copy Markdown
Contributor

What

MSTabs's selected indicator moves from selected:border-color-border to selected:border-primary.

Why

border-color-border is the same token the list slot 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

  • New case in test/ui/components/tabs/tabs_test.dart asserts 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.
  • Full suite green.

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

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@anilcancakir

Copy link
Copy Markdown
Contributor Author

@kodizm

@kodizm

kodizm Bot commented Aug 21, 2026

Copy link
Copy Markdown

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 border-color-border - but the new token skips the alias layer, so the underline lands a shade off every other brand surface, and no [Unreleased] entry went in.

Minor

lib/src/ui/components/tabs/tabs.recipe.dart:25 — maintainability/theming: border-primary is a bare palette utility, not a semantic role. MagicStarterTokens.defaultAliases has no border-color-primary key, and the alias expander matches only a token's bare body (alias_expander.dart: "Alias keys are always BARE ... on a hit the outer prefix is re-applied"), so selected:border-primary passes through untouched and the border parser defaults the missing shade to 500 (border_parser.dart:203, final shade = shadeStr != null ? int.parse(shadeStr) : 500). The bg-primary role is bg-primary-600 dark:bg-primary-500, so in light mode an active tab underlines in primary-500 next to a primary-600 button, and there is no dark: pair contrary to .claude/rules/widgets.md ("always pair light/dark classes"). selected:border-primary-600 dark:selected:border-primary-500, or a new border-color-primary alias role, would track the brand exactly. This is consistent with existing practice in magic_starter_theme.dart (focus:border-primary), so it is a convention question, not a break.

CHANGELOG.md — no [Unreleased] entry for a user-visible colour change. CLAUDE.md's post-change checklist asks for one after ANY source change, and the two most recent feature commits both carried one (cbeec7c exists purely to backfill a skipped doc).

test/ui/components/tabs/tabs_test.dart:96 — the assertion is "more than one distinct bottom-border colour", which pins the indicator away from the rule but not to the brand: changing the token to selected:border-red-500 keeps this test green. expect(bottomBorders, contains(theme primary)) would hold the actual claim in the title.

Tests

The 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

  • flutter test test/ui/components/tabs/tabs_test.dart - 8 passed, including the new case.
  • flutter analyze --no-fatal-infos - "No issues found!".
  • dart format --set-exit-if-changed on both changed files - 0 changed.
  • Full suite not run; both changed files were read in full, and I grepped for MSTabs/tabsRecipe callers in lib/ and found none outside the component's own directory.

@anilcancakir

Copy link
Copy Markdown
Contributor Author

@kodizm review

1 similar comment
@anilcancakir

Copy link
Copy Markdown
Contributor Author

@kodizm review

@kodizm

kodizm Bot commented Aug 21, 2026

Copy link
Copy Markdown

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

CHANGELOG.md — still no [Unreleased] entry for this user-visible colour change (the section is empty at line 5). Flagged last time and unaddressed; CLAUDE.md's post-change checklist asks for one after ANY source change. The other two earlier notes (bare border-primary skipping the alias layer, and the test pinning the indicator away from the rule rather than to the brand) are convention calls, not blockers - fine to leave if that is the deliberate choice.

Tests

test/ui/components/tabs/tabs_test.dart covers the change: it asserts the rendered bottom-border colours of the indicator and the list rule differ. Unchanged since my last read.

Checks I ran

  • git log --oneline — head is 20eb48c, the same commit as the previous review; no new commits on the branch.
  • flutter test test/ui/components/tabs/tabs_test.dart00:00 +8: All tests passed!
  • Read CHANGELOG.md head — ## [Unreleased] section is empty.

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.
@anilcancakir

Copy link
Copy Markdown
Contributor Author

All three verified and fixed.

The bare token. Confirmed exactly as described: no border-color-primary in the alias map (only border-color-border and border-color-border-subtle), so the alias layer passes it through, and border_parser.dart:203 defaults the missing shade to 500 while bg-primary is bg-primary-600 dark:bg-primary-500. Now selected:border-primary-600 dark:selected:border-primary-500, with the dark: prefix first to match the five existing dark:hover: spellings here.

The under-specified assertion. Right, and it was the more useful of the two points: border-red-500 kept it green. It now asserts the indicator IS the brand shade, in light and in dark, and both new assertions fail on the bare token.

Two things I had to measure rather than reason, both of which would have shipped a test that looked fine and proved nothing:

  • The expected colour comes from WindThemeData().colors['primary'], not a hex. My first attempt used Material's Colors.blue.shade600 (#1E88E5); wind's default primary is Tailwind blue, so the indicator actually renders #2563EB.
  • The dark case needs WindThemeData(brightness: dark, syncWithSystem: false). A MediaQuery(platformBrightness: dark) override does not reach wind, which reads its own theme data, so the first version of that case rendered light twice and passed for the wrong reason.

The changelog. Added under [Unreleased] / Fixed, naming both the smudge and the shade that the first fix introduced.

Gates on the tip: flutter test 1255 pass, dart analyze clean, dart format 0 changed.

@kodizm

kodizm Bot commented Aug 21, 2026

Copy link
Copy Markdown

Note

Kodizm (AI-generated). May contain mistakes; verify before acting.

Incremental re-review of 7747804: this commit closes all three of my earlier notes - the token now tracks bg-primary shade for shade in both modes, the dark half is paired, the [Unreleased] entry is in, and the test pins the indicator to the brand rather than merely away from the rule. Nothing new to raise.

What changed since 20eb48c: selected:border-primary became selected:border-primary-600 dark:selected:border-primary-500, a dark-mode test case and a _brandShade() helper were added, and a ### Fixed entry landed under [Unreleased].

Two things I checked rather than assumed:

  • The new shades match the alias exactly. magic_starter_tokens.dart:126 defines 'bg-primary': 'bg-primary-600 dark:bg-primary-500', so the indicator now resolves to the same pair as every other brand surface. This is the right call over adding a border-color-primary alias, since it needs no new token.
  • The dark:selected: chain is sound, and the dark test is not vacuous. WindParser.resolveClasses splits on : and evaluates every prefix as a conjunction with no ordering constraint, so the class needs both dark brightness and the selected state - an unselected tab in dark mode gets no underline. Prefix order is a free choice here, not a latent bug.

Tests

test/ui/components/tabs/tabs_test.dart now asserts both halves: light mode contains _brandShade(600) and the new dark case contains _brandShade(500). Reading the shade off WindThemeData().colors rather than hardcoding a hex is the correct call - it pins to the theme, not to Tailwind's palette. The two shades are distinct colours, so neither assertion can pass by accident if the other variant leaks.

Checks I ran

  • git log --oneline - one new commit, 7747804, on top of the previously reviewed 20eb48c.
  • flutter test test/ui/components/tabs/tabs_test.dart - 00:00 +9: All tests passed! (was 8; the dark case is the ninth).
  • flutter test - 02:30 +1255: All tests passed!
  • flutter analyze --no-fatal-infos - No issues found!
  • dart format --set-exit-if-changed lib test - Formatted 321 files (0 changed).
  • Read WindParser.resolveClasses in fluttersdk_wind-1.4.0 to confirm chained-prefix semantics, and magic_starter_tokens.dart:126 to confirm the bg-primary shade pair.

@anilcancakir
anilcancakir merged commit 443bb60 into main Aug 21, 2026
2 checks passed
@anilcancakir
anilcancakir deleted the fix/tabs-active-indicator branch August 21, 2026 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant