Skip to content

fix(react): #4515 inherit theme colour in slotted SVGs - #4629

Merged
GCHQ-Developer-299 merged 1 commit into
mi6:developfrom
sylvesterkaczmarek:fix/4515-slotted-svg-theme
Aug 27, 2026
Merged

fix(react): #4515 inherit theme colour in slotted SVGs#4629
GCHQ-Developer-299 merged 1 commit into
mi6:developfrom
sylvesterkaczmarek:fix/4515-slotted-svg-theme

Conversation

@sylvesterkaczmarek

@sylvesterkaczmarek sylvesterkaczmarek commented Aug 22, 2026

Copy link
Copy Markdown

Summary of the changes

Make React SlottedSVG icons inherit theme-aware component colours when no explicit fill is supplied.

  • Default the SVG fill to currentColor.
  • Preserve explicit consumer-provided fill values.
  • Add a focused Cypress regression using the reported IcBreadcrumb pattern.
  • Verify the icon's computed fill changes when the wrapping IcTheme changes from light to dark.

Related issue

Closes #4515

Validation

  • Final branch diff audited against develop: two intended React files only.
  • Focused Cypress regression coverage added.
  • Local tests were not run because the repository dependency tree was unavailable in this workspace. GitHub Actions should validate the change after the PR is opened.
  • No public API changes. Existing explicit SVG fill values remain supported.

Checklist

General

  • All acceptance criteria reviewed and met.

Testing

  • Relevant regression coverage added.
  • GitHub Actions completed.

Accessibility

  • Theme-aware icon colour behaviour covered.

Testing content extremes

  • Explicit consumer-provided fill values remain supported.

Comment thread packages/react/src/component-tests/IcBreadcrumb/IcBreadcrumb.cy.tsx Outdated

@GCHQ-Developer-530 GCHQ-Developer-530 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is quite working as expected because currentColor would make the svgs the link blue colour instead of black/white.

Image

Also in order to get this running in storybook I had to change the Icon story for breadcrumb to use SlottedSVG, so while making the change to SlottedSVG is probably the correct thing to do, in order to test it you'll need to make those changes too on lines 164, 177, and 190 of ic-breadcrumb-group.stories.jsx in packages/react

@sylvesterkaczmarek

Copy link
Copy Markdown
Author

Updated. SlottedSVG now defaults to the theme-aware neutral foreground token rather than currentColor, so breadcrumb icons use the appropriate light/dark foreground instead of inheriting the link colour.

I also updated the Breadcrumb Icon Storybook example to use SlottedSVG and tightened the Cypress regression to assert the resolved light and dark fill colours. The branch has also been rebuilt cleanly on current develop.

@GCHQ-Developer-530
GCHQ-Developer-530 force-pushed the fix/4515-slotted-svg-theme branch 3 times, most recently from 57214a8 to 3caeb87 Compare August 26, 2026 14:21
@GCHQ-Developer-530
GCHQ-Developer-530 force-pushed the fix/4515-slotted-svg-theme branch from 3caeb87 to 0b1cbba Compare August 27, 2026 10:46
@GCHQ-Developer-530
GCHQ-Developer-530 requested a review from a team August 27, 2026 11:35

@GCHQ-Developer-299 GCHQ-Developer-299 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally with the updated story with/without the change to slottedSVG and confirmed that this works :)

@GCHQ-Developer-299
GCHQ-Developer-299 merged commit 255aadb into mi6:develop Aug 27, 2026
6 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.

4 participants