ENG-2890: Extend message and notification APIs to use Carbon icons#7582
Merged
gilluminate merged 24 commits intomainfrom Mar 9, 2026
Merged
ENG-2890: Extend message and notification APIs to use Carbon icons#7582gilluminate merged 24 commits intomainfrom
gilluminate merged 24 commits intomainfrom
Conversation
Wrap the useModal hook in FidesUIProvider to inject Carbon icon equivalents (InformationFilled, CheckmarkFilled, WarningFilled, Misuse) as defaults for info, success, warning, error, and confirm modals. Callers can still override with icon prop or suppress with icon: null. Add Modal Methods Storybook stories for visual verification. ENG-2889 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add useEffect dependencies and destroy() cleanup in stories - Use public import path for CarbonIconType - Add hideIcon default arg to all stories, remove standalone NoIcon story Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add Carbon icon defaults for message (16px) and notification (24px) APIs, matching the existing modal pattern. Wrap both APIs in FidesUIProvider to automatically inject icons for info/success/error/warning methods while preserving loading's default spinner. Migrate admin-ui call sites from direct antd notification usage to the provider's useNotification hook. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis PR extends message and notification APIs to use Carbon icons via the FidesUI provider. It refactors carbon-icon-defaults to support message and notification feedback types with corresponding icon maps, updates FidesUIProvider to wrap message and notification methods with automatic default icons, and modifies components to use the new useNotification hook-based API instead of direct notification imports. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
Greptile SummaryThis PR extends the Carbon icon injection pattern (previously applied only to Ant Design's
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 241a318 |
speaker-ender
approved these changes
Mar 6, 2026
Contributor
speaker-ender
left a comment
There was a problem hiding this comment.
Overall looks good. Left some comments but don't need to be addressed now.
Comment on lines
+29
to
+51
| const wrapMessageMethod = ( | ||
| method: MessageTypeOpen, | ||
| type: string, | ||
| ): MessageTypeOpen => { | ||
| const defaultIcon = getDefaultMessageIcon(type); | ||
| return (content, duration?, onClose?) => { | ||
| if (isMessageArgsProps(content)) { | ||
| // ArgsProps form - inject icon as default, caller can override | ||
| return method({ icon: defaultIcon, ...content }); | ||
| } | ||
| // JointContent (string/ReactNode) - convert to ArgsProps to inject icon | ||
| const args: MessageArgsProps = { content, icon: defaultIcon }; | ||
| if (typeof duration === "number") { | ||
| args.duration = duration; | ||
| } else if (typeof duration === "function") { | ||
| args.onClose = duration; | ||
| } | ||
| if (onClose) { | ||
| args.onClose = onClose; | ||
| } | ||
| return method(args); | ||
| }; | ||
| }; |
Contributor
There was a problem hiding this comment.
Any thoughts on just picking one method for using messages?
It's a little much to expect devs to pick between the two.
If we leave in we should leave a recommendation/explanation of when to use each.
Contributor
Author
There was a problem hiding this comment.
I think it's fine to keep both. Devs should be getting direction from design on which feedback pattern to use for a given interaction, so the choice shouldn't fall on them to figure out independently.
Contributor
There was a problem hiding this comment.
@gilluminate To be clear I'm talking about the two styles of function calls
message.success(content, [duration], onClose)message.success(config)
This is more of a technical/code style decision imo
Contributor
Author
There was a problem hiding this comment.
Ah my bad. That makes more sense, I thought you were talking about only using message OR notification. Hmmmm...yeah, I'm torn on this one. Notification ONLY supports the config style, so it kind of makes sense to restrict to only use that for message. On the other hand, we have 70+ instances of message only using the content style so that'd be a major overhaul. Keeping both in the wrapper is ~15 lines of logic to avoid touching 70+ files.
Contributor
Author
There was a problem hiding this comment.
Added a JSDoc to wrapMessageMethod recommending the string form as the default. Happy to collapse to one style if we find a strong reason to later.
- Type icon map keys with FeedbackType and ModalType unions - Remove nullish coalescing from getter functions - Update wrapMessageMethod to use FeedbackType param Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wrap the useModal hook in FidesUIProvider to inject Carbon icon equivalents (InformationFilled, CheckmarkFilled, WarningFilled, Misuse) as defaults for info, success, warning, error, and confirm modals. Callers can still override with icon prop or suppress with icon: null. Add Modal Methods Storybook stories for visual verification. ENG-2889 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add useEffect dependencies and destroy() cleanup in stories - Use public import path for CarbonIconType - Add hideIcon default arg to all stories, remove standalone NoIcon story Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove unnecessary modalApi spread in FidesUIProvider - Nest modal method stories under Feedback/Modal/Methods - Add Primary story pattern with type control disabled on variants Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
cbbe49d to
2666242
Compare
…ssage-notification-carbon-icons
51ba03f to
ef5cb78
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
7 tasks
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/fidesui/src/FidesUIProvider.tsx`:
- Around line 46-48: The spread `{ icon: defaultIcon, ...content }` lets callers
pass icon: undefined and overwrite the injected default; change the logic in the
isMessageArgsProps branch to set icon only when the incoming icon is undefined
(e.g., determine icon = content.icon === undefined ? defaultIcon : content.icon)
and pass that computed icon into method instead of relying on the spread, and
apply the exact same undefined-only check to the success, error, and warning
notification handlers so null remains a valid explicit opt-out while undefined
preserves the defaultIcon.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 104fec4b-af2a-4d0c-9b15-a856646f9c96
📒 Files selected for processing (5)
changelog/7582-message-notification-carbon-icons.yamlclients/admin-ui/src/features/privacy-assessments/AssessmentTaskStatusIndicator.tsxclients/admin-ui/src/features/privacy-experience/preview/Preview.tsxclients/fidesui/src/FidesUIProvider.tsxclients/fidesui/src/lib/carbon-icon-defaults.tsx
Comment on lines
+46
to
+48
| if (isMessageArgsProps(content)) { | ||
| // ArgsProps form - inject icon as default, caller can override | ||
| return method({ icon: defaultIcon, ...content }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '44,49p' clients/fidesui/src/FidesUIProvider.tsx
sed -n '123,141p' clients/fidesui/src/FidesUIProvider.tsx
node <<'NODE'
const cases = [
{
label: 'message/object form',
value: { icon: 'default', ...{ icon: undefined, content: 'Saved' } },
},
{
label: 'notification/object form',
value: { icon: 'default', ...{ icon: undefined, message: 'Saved' } },
},
{
label: 'explicit null opt-out',
value: { icon: 'default', ...{ icon: null, message: 'Saved' } },
},
];
for (const { label, value } of cases) {
console.log(label, value);
}
NODERepository: ethyca/fides
Length of output: 1242
Don't let icon: undefined overwrite the injected default.
In { icon: defaultIcon, ...content } and the notification handlers, when callers pass an optional icon field that resolves to undefined, the spread operator overwrites the default icon. This breaks the automatic icon injection behavior. Use an explicit undefined check to preserve the default only when icon is not explicitly provided, while still allowing null as an intentional opt-out.
🛠️ Suggested change
if (isMessageArgsProps(content)) {
// ArgsProps form - inject icon as default, caller can override
- return method({ icon: defaultIcon, ...content });
+ return method({
+ ...content,
+ icon: content.icon === undefined ? defaultIcon : content.icon,
+ });
} info: (props: Parameters<typeof notificationApi.info>[0]) =>
notificationApi.info({
- icon: getDefaultNotificationIcon("info"),
...props,
+ icon:
+ props.icon === undefined
+ ? getDefaultNotificationIcon("info")
+ : props.icon,
}),Apply the same pattern to success, error, and warning notification handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/fidesui/src/FidesUIProvider.tsx` around lines 46 - 48, The spread `{
icon: defaultIcon, ...content }` lets callers pass icon: undefined and overwrite
the injected default; change the logic in the isMessageArgsProps branch to set
icon only when the incoming icon is undefined (e.g., determine icon =
content.icon === undefined ? defaultIcon : content.icon) and pass that computed
icon into method instead of relying on the spread, and apply the exact same
undefined-only check to the success, error, and warning notification handlers so
null remains a valid explicit opt-out while undefined preserves the defaultIcon.
12 tasks
mfbrown
pushed a commit
that referenced
this pull request
Mar 12, 2026
…7582) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
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
2 participants
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.
Ticket ENG-2890
Description Of Changes
Extend the Carbon icon wrapping pattern from #7569 to cover Ant Design's
messageandnotificationAPIs. All three feedback APIs (modal, message, notification) now automatically inject Carbon icons forinfo,success,error, andwarningmethods when consumed through the FidesUI provider hooks.The message API's
loadingmethod intentionally keeps Ant's default spinner, andopen/destroyare inherited unchanged.Also migrates two admin-ui components that were using Ant's
notificationdirectly to go through the provider'suseNotification()hook, ensuring consistent icon rendering.Code Changes
clients/fidesui/src/lib/carbon-icon-defaults.tsx- Add message (16px + 8px margin) and notification (24px) icon maps with getter functionsclients/fidesui/src/FidesUIProvider.tsx- WrapmessageApiandnotificationApiwith Carbon icon defaults; handle message API's dual call signature (string vs object form)clients/admin-ui/src/features/privacy-assessments/AssessmentTaskStatusIndicator.tsx- Migrate fromnotification.useNotification()to provider'suseNotification()hook; remove Fragment wrapperclients/admin-ui/src/features/privacy-experience/preview/Preview.tsx- Migrate from staticnotification.warning()to provider'suseNotification()hookSteps to Confirm
clients/fidesui/(npm run storybook)npm run devinclients/admin-ui/)Pre-Merge Checklist
CHANGELOG.mdupdatedSummary by CodeRabbit
New Features
Refactor