Skip to content

Recolor primary-button icons from token via mask, not invert - #90

Merged
thomasgallagher merged 2 commits into
mainfrom
fix/primary-button-icon-token-recolor
Jun 7, 2026
Merged

Recolor primary-button icons from token via mask, not invert#90
thomasgallagher merged 2 commits into
mainfrom
fix/primary-button-icon-token-recolor

Conversation

@thomasgallagher

Copy link
Copy Markdown
Collaborator

The .l-ui-button--primary .l-ui-icon rule used a binary invert filter, which can only produce black or white. Accents with a non-white foreground rendered the wrong icon colour and had to be patched out downstream with filter: none.

Paint the icon from the existing --button-primary-icon token using a CSS mask (the technique already used for the select dropdown arrow), with a higher-specificity .dark override so the base dark:invert can't flip the token colour. The panel hide button - the only such call site - now emits the SVG as --l-ui-icon-src on a span instead of an img src.

The broadly-used .l-ui-icon img + dark:invert behaviour is unchanged.

The .l-ui-button--primary .l-ui-icon rule used a binary invert filter,
which can only produce black or white. Accents with a non-white
foreground rendered the wrong icon colour and had to be patched out
downstream with filter: none.

Paint the icon from the existing --button-primary-icon token using a CSS
mask (the technique already used for the select dropdown arrow), with a
higher-specificity .dark override so the base dark:invert can't flip the
token colour. The panel hide button - the only such call site - now
emits the SVG as --l-ui-icon-src on a span instead of an img src.

The broadly-used .l-ui-icon img + dark:invert behaviour is unchanged.
@claude

claude Bot commented Jun 7, 2026

Copy link
Copy Markdown

Review

Replaces the binary invert filter on primary-button icons with a CSS mask painted from the --button-primary-icon token, and migrates the only call site (the panel hide button) from image_tag to a <span> carrying the SVG path as a custom property.

Issues

Severity File Line(s) Description
MEDIUM _panel.html.erb 44 asset_path output is interpolated directly into the inline style attribute without CSS url() quotes. While the fingerprinted path for a static engine asset will never contain spaces or parens in practice, the unquoted form is fragile. If the path ever contains a space (e.g. a future rename, a CDN prefix, or a host-app asset-host with a space-encoded URL) the CSS is silently broken with no browser error. Prefer the quoted form: url('<%= asset_path "layered_ui/icon_panel_close.svg" %>').

Verdict

🟧 Ship (with known minor issues) - The core mask approach is correct and the specificity layering is sound; the only finding is a robustness nit on the unquoted url().

@claude

claude Bot commented Jun 7, 2026

Copy link
Copy Markdown

Review

Replaces the binary invert filter on primary-button icons with a CSS mask driven by the --button-primary-icon token, and migrates the only engine call site (panel hide button) from <img> to a <span style="--l-ui-icon-src: url(...)">.

Issues

Severity File Lines Description
MEDIUM CHANGELOG.md 9 The entry is filed under ### Fixed and says only that downstream filter: none overrides can be removed. It does not document the breaking behaviour change for any downstream <img class="l-ui-icon"> elements inside .l-ui-button--primary. Under the old rule those images were invert-ed white in light mode; under the new rule the invert is gone and the mask approach requires --l-ui-icon-src to be set on the element, so unmodified <img> icons will render in their original (typically dark) colour against the dark primary-button background. The engine's only call site is correctly migrated here, but the changelog should either move this to ### Changed with a Breaking label or add a migration note telling consumers to swap <img class="l-ui-icon"> inside primary buttons to <span style="--l-ui-icon-src: url(...)" class="l-ui-icon">.

Verdict

🟧 Ship (with known minor issue) - The CSS and view changes are technically correct; the only concern is that the migration path for downstream <img> icons in primary buttons is undocumented.

@thomasgallagher
thomasgallagher merged commit bf6c1f1 into main Jun 7, 2026
4 checks passed
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