Skip to content

fix(banner): status icon inherits color from the banner-icon wrapper so 'status:X' theme overrides reach it#4201

Open
jiunshinn wants to merge 1 commit into
facebook:mainfrom
jiunshinn:fix/4166-banner-info-icon
Open

fix(banner): status icon inherits color from the banner-icon wrapper so 'status:X' theme overrides reach it#4201
jiunshinn wants to merge 1 commit into
facebook:mainfrom
jiunshinn:fix/4166-banner-info-icon

Conversation

@jiunshinn

Copy link
Copy Markdown
Contributor

Problem

Banner renders its status="info" icon as <Icon color="accent">. The documented theming target astryx-banner-icon (+ 'status:info') styles the wrapper div only — the inner .astryx-icon span carries its own data-color="accent" StyleX color (color: var(--color-accent)), so a 'banner-icon' override that sets color never reaches the glyph. On themes whose accent is not a calm blue (the reporter's accent is #DC2626 red), every info banner ships a red (i) icon that is indistinguishable from an alert, with no recovery path inside the theme DSL.

Root cause

packages/core/src/Banner/Banner.tsx mapped each status to an Icon color variant and passed it down:

const statusIconColor = {
  info: 'accent',
  warning: 'warning',
  error: 'error',
  success: 'success',
} as const;
...
<Icon icon={defaultIconName} size="md" color={iconColor} />

Icon then sets color: var(--color-accent) directly on the .astryx-icon span. Since theme component overrides for 'banner-icon' (emitted into @layer astryx-theme) target the wrapper element, and the span's own color declaration beats inheritance, the override can restyle the wrapper but the SVG (stroke/fill: currentColor) keeps reading the span's hard-coded color.

Fix — option (a): glyph inherits from the banner-icon wrapper

The issue offered two directions:

  • (a) the status icon uses currentColor / inherits from .astryx-banner-icon so 'status:info' overrides reach it — chosen
  • (b) info resolves through a dedicated token (e.g. --color-info, accent as fallback) that themes can set — not chosen here

Implementation: the status color default moves onto the wrapper (the 'banner-icon' theming target) via StyleX, using exactly the tokens the Icon variants resolved to before, and the default Icon now renders color="inherit":

const statusIconColorStyles = stylex.create({
  info: {color: colorVars['--color-accent']},
  warning: {color: colorVars['--color-warning']},
  error: {color: colorVars['--color-error']},
  success: {color: colorVars['--color-success']},
});
...
stylex.props(styles.iconWrapper, icon == null && statusIconColorStyles[status])
...
<Icon icon={defaultIconName} size="md" color="inherit" />

Why (a) over (b):

  • Default visuals are pixel-identical. The wrapper carries color: var(--color-accent) (info) / the semantic status tokens (others) — the same vars the Icon color variants used — and the glyph inherits through currentColor. No theme, light/dark mode, or default render changes.
  • (b) is a real API addition — a new --color-info token needs entries in tokens.stylex.ts, the HCT palette derivation for custom themes, an Icon color union member, and docs across Icon/tokens. The issue itself says the --color-info idea "might deserve … a separate issue". (a) does not preclude (b): if a --color-info token lands later, statusIconColorStyles.info switches to it in one line.
  • All in-repo themes keep working unchanged. neutral/butter/gothic/stone/y2k retheme banner icons by re-scoping the tokens on the banner root (e.g. neutral: 'status:info': {'--color-accent': 'var(--color-text-blue)'}). The wrapper resolves the same var() inside the same subtree, so those overrides produce identical results.
  • Applied to all four statuses, not just info, so the documented astryx-banner-icon + data-status theming surface actually controls the glyph for every status instead of silently styling an invisible wrapper.
  • The wrapper color is applied only for the default icon (icon == null), so a custom icon node keeps its current inherited color — no behavior change on that path.

Before / after

Before (core 0.1.2, from the issue):

<div class="astryx-banner-icon info ...">                       <!-- override applies here -->
  <span class="astryx-icon md accent ..." data-color="accent">  <!-- color: var(--color-accent), unreachable -->
    <svg fill="currentColor" ...>

After (rendered by the updated test suite):

<div class="astryx-banner-icon info ... xqwr325" data-status="info">   <!-- .xqwr325 { color: var(--color-accent) } -->
  <span class="astryx-icon md inherit ..." data-color="inherit">        <!-- color: inherit -->
    <svg fill="currentColor" ...>

A theme's 'banner-icon': {'status:info': {color: '#67E8F9'}} override lands on the wrapper in @layer astryx-theme (above StyleX), and the glyph now follows it via inheritcurrentColor. With no override, the computed color is var(--color-accent) exactly as before.

Test plan

  • pnpm exec vitest run packages/core/src/Banner — 36 passed (33 existing + 3 new)
  • New tests pin: (1) every status renders the default icon with data-color="inherit"; (2) the info icon no longer carries data-color="accent" (regression pin); (3) the wrapper color class is applied only on the default-icon path, so custom icons keep their inherited color. All three fail against the previous implementation.
  • pnpm -F @astryxdesign/core typecheck — clean
  • eslint src/Banner — clean
  • Verified .xqwr325color: var(--color-accent) and .x1heor9gcolor: inherit in the built astryx.css, confirming the unchanged default color chain.

Related (out of scope)

Badge variant="info" has the same accent-fallback semantics, but is recoverable today via 'variant:info' overrides, so it is not touched here. If a --color-info token is introduced (issue's option (b)), both Banner and Badge could resolve through it as a follow-up.

Reviewer questions

  1. (a) vs (b): This PR implements option (a) (inheritance from the banner-icon wrapper). If you'd rather go straight to option (b) (--color-info token with accent fallback, plus the palette-derivation and Icon changes it implies), happy to rework — (a) was chosen as the minimal fix that keeps default rendering byte-identical and leaves the token design open.
  2. Scope: the inheritance change is applied to all four statuses so 'banner-icon' + 'status:X' overrides work uniformly, not just for info. If you prefer an info-only change (keeping data-color="warning|error|success" on the sibling icons), that is a two-line narrowing.
  3. Observable DOM note: the inner .astryx-icon span's data-color changes from the status variant to "inherit". Anything targeting e.g. .astryx-icon[data-color="accent"] inside banners (outside the documented theming surface) would stop matching — flagged in case that surface matters to you.

Fixes #4166

@vercel

vercel Bot commented Jul 22, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
astryx Ready Ready Preview, Comment Jul 22, 2026 3:11pm

Request Review

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Jul 22, 2026
@github-actions github-actions Bot added community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge needs:design-review Affects visuals — Design should review labels Jul 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR Analysis Report

Modified Components

Banner
Metric Before After Delta
Bundle Size (ESM) N/A N/A N/A
Lines of Code N/A 332 -
Complexity N/A High (23) -

Bundle Size Summary

Package Size (ESM) Size (CJS) Gzipped
@astryxdesign/core N/A 4.7KB 0B

Accessibility Audit

Status: 1 accessibility violation(s) found — 1 serious.

Banner - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 1/15 stories · Learn more
    • WCAG: 1.4.3 (Level AA)

Generated by PR Enrichment workflow | View full report

@jiunshinn
jiunshinn marked this pull request as ready for review July 22, 2026 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot. community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge needs:design-review Affects visuals — Design should review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Banner: info-status icon is hard-coded to Icon color="accent", unreachable by 'status:info' theme overrides

1 participant