Adopt the design-system workflow icons and make them theme-aware - #34142
Open
anuj-kumary wants to merge 4 commits into
Open
anuj-kumary wants to merge 4 commits into
anuj-kumary wants to merge 4 commits into
Conversation
anuj-kumary
requested review from
ShaileshParmar11,
chirag-madlani and
karanh37
as code owners
September 28, 2026 11:22
anuj-kumary
added
safe to test
Contributor
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
Contributor
Jest test CoverageUI tests summary
|
The workflow node and control icons baked literal hex, so tw:text-* on them
was inert and they never followed the theme - ic_undo/ic_redo at #545A84 were
close to invisible on the dark control bar, and the start/end discs stayed
light-tinted.
Design supplied 12 single-tone glyphs (#717680, 20x20). They go through the
core library's regular icon lane, whose replaceHardcodedColors pass rewrites
every hex fill/stroke to currentColor and whose SVGR config injects
stroke={color}. So they are themeable by construction, with no hand-editing:
icons/workflow-*.svg -> yarn icons:generate -> src/icons/Workflow*.tsx
-> src/icons/index.ts
NodeIconUtils and CustomControls now import from
@openmetadata/ui-core-components/icons, and the 20 replaced assets are gone.
ic_star.svg stays as the node fallback - design has not supplied one.
Two things the first cut got wrong, both caught by rendering it:
Size. The call sites drew start/end at 32px and task nodes at 16px. That
balanced before because the old start/end assets were 32px discs holding a
~16px glyph; design ships bare glyphs, so 32px rendered them double-size. All
four call sites are 16px now.
Colour. Dropping the baked hex also dropped the canvas's per-node-type colour
coding. Each subtype now maps to the hue its old icon carried, through utility
tokens that flip:
start #067647 -> #47cd89 action #2e90fa -> #53b1fd
end #b42318 -> #f97066 approval #dd2590 -> #f670c7
check #b54708 -> #fdb022
The -600 shades looked right but are pinned: UntitledUIThemeProvider writes
--tw-color-utility-{success,error,warning,brand}-600 as inline styles on
<html> from the tenant theme, so they hold one value in both themes. Measuring
on the sign-in page hides this because the provider has not run yet; measured
after login, -500 and -700 flip and -600 does not.
Icons default to text-quaternary, whose light value is #717680 - design's
exact export - so an unmapped node keeps their tone.
anuj-kumary
force-pushed
the
workflow-icons
branch
from
September 28, 2026 11:52
5ff00ad to
404dc06
Compare
Review feedback: the icons carried style={{ width, height }}.
A size class cannot simply be concatenated here. The getters supply a default
and nine call sites override it, so 'tw:size-8' and 'tw:size-4' would both land
on the element and Tailwind's own stylesheet order - not the class attribute -
would pick the winner. cx/tailwind-merge is not exported from the core package,
so there is nothing to dedupe them.
The getters now take a size token and emit exactly one class, which removes the
collision:
ICON_SIZE_CLASS = { sm: 'tw:size-4', md: 'tw:size-8' }
getNodeIcon(subType, { size: 'sm' })
md stays the default because the knowledge graph (CustomNode,
KnowledgeGraphInspectorParts) calls these with no options and relies on 32px.
Both getters now share one renderNodeIcon helper rather than repeating the
prop-merging, and className still composes so a caller can add to it.
Verified on the canvas: icons render with style absent and
class="size-4 text-utility-success-700", 16px as before.
Review feedback: the exported files carried width="20" height="20" on the root
<svg>.
They were inert - generate-icons.mjs strips the root width/height and SVGR
re-injects width={size} height={size} - and all 189 existing source icons carry
them the same way, so ours matched the library. But they read as hardcoded
sizing in the diff, which is the opposite of the point.
Removed from all 12. Regenerating produces byte-identical components, which
confirms the attributes never reached the output.
Code Review ✅ Approved🟡 Medium risk · Workflow canvas icons now use theme-aware glyphs and subtype color tokens. Adopts design-system workflow icons and makes them theme-aware by routing all 12 glyphs through the icon library's OptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
Contributor
|
| Count | Rule |
|---|---|
| 1 | no-restricted-imports |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | src/utils/NodeIconUtils.tsx:29:1 |
no-restricted-imports |
'../assets/svg/ic_star.svg' import is restricted from being used by a pattern. Do not import SVG icons directly from assets/ paths; use the designated abstracti |
Fix locally (fast - only checks files changed in this branch):
make ui-checkstyle-changed
|
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes:
The workflow node and control icons baked literal hex, so
tw:text-*on them was inert and they never followed the theme.ic_undo.svg/ic_redo.svgat#545A84were close to invisible on the dark control bar, and the start/end discs stayed light-tinted. This was the known follow-up left open when #34003 merged.Design supplied 12 single-tone glyphs (
#717680, 20×20), which I added to the core icon library rather than dropping intoassets/svg.Why the library lane matters.
icons/runs areplaceHardcodedColorspass that rewrites every hexfill/stroketocurrentColor, and its SVGR config injectsstroke={color}. So these are themeable by construction — no hand-editing of SVGs:NodeIconUtils.tsxandCustomControls.tsxnow import from@openmetadata/ui-core-components/icons, and the 20 replaced assets are deleted.ic_star.svgstays as the node fallback — design hasn't supplied one.Naming follows the library's existing convention: kebab-case source files in a prefixed family, exactly like the existing
data-*(7),total-*(4) andtest-*(4) groups.Two things the first cut got wrong, both caught by rendering it
Size. The call sites drew start/end at 32px and task nodes at 16px. That balanced before because the old start/end assets were 32px discs holding a ~16px glyph; design ships bare glyphs, so 32px rendered them double-size. All four call sites are 16px now.
Colour. Dropping the baked hex also dropped the canvas's per-node-type colour coding. Each subtype now maps to the hue its old icon carried, through utility tokens that flip:
#067647#47cd89#b42318#f97066#b54708#fdb022#2e90fa#53b1fd#dd2590#f670c7Icons default to
tw:text-quaternary, whose light value is#717680— design's exact export — so an unmapped subtype keeps their tone.Type of change:
High-level design:
N/A — icon-library additions plus two consuming components.
Tests:
Use cases covered
Unit tests
npx jest src/components/WorkflowDefinitions src/utils/NodeIcon→ 99 passed, 12 suites.Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
/workflows/AIAssetApprovalWorkflow/workflow(12 nodes) and read computedstrokeand rendered size off each node icon, in both themes and after login so the theme provider is active.Icons.stories.tsx, which enumeratessrc/icons/index.tsautomatically — no story changes needed.Gates:
yarn lint0 errors ·yarn tw-audit0 errors · tsc clean for the changed files.UI screen recording / screenshots:
Note for anyone pulling this branch
After core-components is rebuilt, Vite serves a stale pre-bundled dep copy and the page dies with
does not provide an export named 'WorkflowCheckConditions'. Clear it:Known follow-ups (not in this PR)
ic_click.svg(#4C526C, empty-canvas message),ic_add.svg, andic_star.svg(the node fallback), plusworkflow.svgin the page header.DataCompletenesswas lime#AAC436and maps to the same green as Start, because the token scale has no lime. If design has specific per-node-type colours, those should win.Checklist:
Fixes #<issue-number>above.🤖 Generated with Claude Code