Consolidate custom icons to Carbon design system equivalents#7738
Consolidate custom icons to Carbon design system equivalents#7738gilluminate merged 30 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
There was a problem hiding this comment.
Code Review — Icon Audit Migration (PR #7738)
This is a solid, well-scoped cleanup. The migration from createIcon/createChakraIcon to either Carbon Icons.* equivalents or plain React.forwardRef SVGs is consistent throughout, and the consolidation of brand logos under common/logos/ with a uniform size prop API is a clear improvement. No correctness or security issues. Findings are all suggestions/visual checks.
Suggestions
OktaLogo.tsx — duplicate clipPath ID (flagged inline)
The <clipPath id="clip0_3116_111154"> is a hardcoded static ID. If multiple OktaLogo instances appear on the same page simultaneously (e.g., a table with multiple Okta rows), the DOM will have duplicate IDs. All other logo components in this PR avoid this issue. Worth fixing with useId() or by dropping the clip entirely if the paths are already correctly bounded.
inputs.tsx — dropped boxSize="full" from password toggle icon (flagged inline)
The old EyeIcon was explicitly sized to fill its container (boxSize="full"). The new Carbon icons have their own default dimensions. Verify the icon renders correctly at the small button size.
[id]/index.tsx — Integrations button lost size="small" (flagged inline)
The size="small" prop was removed from the Integrations tab button alongside the icon swap. If this is an intentional UX change, fine — but if not, it's a visual regression bundled into this icon-only PR.
SlackLogo.tsx — default size increased from 20→24px (flagged inline)
Call sites that don't pass an explicit size will get a slightly larger icon than before.
EditDrawer.tsx — TrashCanOutlineIcon fontSize="small" dropped (flagged inline)
Icons.TrashCan renders at its default Carbon size. Quick visual check recommended.
Nice to Have
-
Logo accessibility: The new components in
common/logos/(AWSLogo, OktaLogo, SlackLogo, etc.) don't accept anaria-labelor include a<title>element, and don't spread...propsto the<svg>. This makes it impossible for screen readers to identify the logo. Thefidesuiicon rewrites (ArrowDownRight,Compass,ManualSetup,Monitor) do spread...propsviaforwardRef, which allows callers to passaria-label. Consider aligning the logo components to the same pattern, or at minimum addingrole="img"andaria-labelat the usage sites where they appear without accompanying text. -
Logo export style inconsistency:
AWSLogo,OktaLogo,SlackLogo,TwilioLogouse inline arrow function exports (export const Foo = ...) withoutReact.forwardRef, whileAwsSesLogoandMailgunLogo(renames of the oldAwsIcon/MailgunIcon) are function declarations that also don't forward refs. The fidesui icon rewrites all useforwardRef. Brand logos likely don't need ref forwarding, but a consistent pattern would be cleaner.
Overall this is a well-executed migration — the deletions are justified, the Carbon replacements are appropriate, and the forwardRef rewrites for the remaining custom icons are a clear improvement over createChakraIcon.
speaker-ender
left a comment
There was a problem hiding this comment.
Minor nit/comment but approving as is.
Looking good!
| export const Gallery: Story = { | ||
| render: () => ( | ||
| <List | ||
| dataSource={icons} | ||
| renderItem={(item) => ( | ||
| <List.Item> | ||
| <Flex align="center" gap={16}> | ||
| <item.component size={24} /> | ||
| <Typography.Text code>{item.name}</Typography.Text> | ||
| </Flex> | ||
| </List.Item> | ||
| )} | ||
| /> | ||
| ), | ||
| }; |
There was a problem hiding this comment.
Since we are already making stories for each individual icon, I think this might be unnecessary.
I do like the idea of seeing them all in one place.
That is where the docs plugin will start coming into play (once added)
There was a problem hiding this comment.
This one is for custom icons. There'll be another gallery for all the icons. Once everything is merged in we can take another look and figure out what makes sense. I almost think the individual icon stories are what should go away.
Adds a gallery story showcasing all 5 custom SVG icons (ArrowDownRightIcon, CompassIcon, ManualSetupIcon, MonitorIcon, SparkleIcon) with individual stories that have interactive size controls. https://claude.ai/code/session_01N1P256QmnMwTeWAH6GeJcC
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7509885 to
d9c3216
Compare
Ticket ENG-3081
Description Of Changes
Audit and migrate custom icon components to their Carbon design system (
Icons.*) equivalents, removing ~25 unused or redundant icon components. Brand logos (AWS, Okta, Slack, Twilio, Mailgun) are consolidated underadmin-ui/features/common/logos/with a consistentsizeprop API. The remaining custom SVGs that have no Carbon equivalent (ArrowDownRight, Compass, ManualSetup, Monitor, Sparkle) are rewritten as plainforwardRefcomponents in fidesui, dropping the ChakracreateIcondependency.Icon migration PR 1 of 2
Code Changes
GreenCheckCircleIcon->Icons.CheckmarkFilled,TrashCanSolidIcon->Icons.TrashCan,EyeIcon->Icons.View,MoreIcon->Icons.OverflowMenuVertical, etc.)features/messaging/icons/,features/common/Icon/,features/privacy-assessments/) into a singlefeatures/common/logos/directory with named exports and consistentsizepropsReact.forwardRefcomponents withdisplayName,currentColorsupport, and asizeprop -- seeclients/fidesui/src/icons/features/common/Icon/directory (Globe, database icons, arrow icons, etc.) andfeatures/common/HorizontalStepper.tsxwhich depended on removed iconsclients/fidesui/src/icons/icons.stories.tsx/poc/icon-migrationfor designer review of remaining non-Carbon iconsSample migration sites for review:
FidesTable.tsx(Icons.OverflowMenuVertical,Icons.ExpandAll,Icons.CollapseAll)inputs.tsx(Icons.View/Icons.ViewOff)AwsSesMessagingForm.tsx(AwsSesLogo +Icons.CheckmarkFilled)AddSystem.tsx(AWSLogo, OktaLogo, ManualSetupIcon)[id]/index.tsx(Icons.Settingsreplacing GearLightIcon)Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works