Fix Alert banner variant to use Carbon icons#7627
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Mirror Ant's internal banner defaults (showIcon=true, type="warning") in the CustomAlert HOC so Carbon icons are injected for banner mode. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16b0a6e to
6fdb29a
Compare
Greptile SummaryThis PR fixes the Changes:
The implementation is clean, well-reasoned, and handles all relevant edge cases correctly. Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 6fdb29a |
speaker-ender
left a comment
There was a problem hiding this comment.
Approving as is but I'm leaning towards ignoring ant's defaults since they don't have a clear rationale imo.
| ({ showIcon, icon, type, banner, description, ...props }, ref) => { | ||
| // Mirror Ant's internal defaults so Carbon icons apply correctly: | ||
| // - banner mode defaults type to "warning" (not "info") | ||
| // - banner mode implicitly enables showIcon | ||
| const resolvedType = type ?? (banner ? "warning" : "info"); | ||
| const resolvedShowIcon = showIcon ?? !!banner; | ||
|
|
There was a problem hiding this comment.
I'm open to disagreeing with ant's implementation here and choosing our own sane defaults since we are wrapping this.
It seems odd and unintuitive that the default type and showIcon values are different based on the banner prop.
There was a problem hiding this comment.
I like your thinking! Simplified so all alert types behave consistently - type defaults to "info" and showIcon defaults to false across the board, overriding Ant's banner-specific defaults (type="warning", showIcon=true). No existing banner usages in the codebase, so this is a safe change.
Override Ant's banner-specific defaults so all alert types behave identically (type="info", showIcon=false). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ticket ENG-2891
Description Of Changes
Fix the Alert
bannervariant to use Carbon icons instead of Ant's built-in icons. The initial Carbon icon integration (PR #7613) missed the banner case because Ant'sbannerprop implicitly enables icon display and defaults totype="warning"internally, but our HOC was gating icon injection on the explicitshowIconprop and defaultingtypeto"info".This mirrors Ant's internal banner defaults in the CustomAlert HOC:
showIconresolves totruewhenbanneris set andshowIconisn't explicitly providedtyperesolves to"warning"whenbanneris set andtypeisn't explicitly providedCode Changes
clients/fidesui/src/hoc/CustomAlert.tsx- Destructurebannerprop; computeresolvedTypeandresolvedShowIconmirroring Ant's internal defaults; use resolved values for Carbon icon injection and prop passthrough; update JSDocSteps to Confirm
cd clients/fidesui && npm run storybooktype="info"shows Carbon InformationFilledshowIcon={false}hides icon entirelyPre-Merge Checklist
CHANGELOG.mdupdated