ENG-2889: Replace Ant default icons with Carbon icons in Modal methods#7569
Open
gilluminate wants to merge 9 commits intomainfrom
Open
ENG-2889: Replace Ant default icons with Carbon icons in Modal methods#7569gilluminate wants to merge 9 commits intomainfrom
gilluminate wants to merge 9 commits intomainfrom
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWraps Ant Modal imperative methods with defaults to inject Carbon icons; adds a shared icon utility and Storybook stories demonstrating each modal method. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.5)
participant Consumer as Component (useModal)
end
rect rgba(220,255,220,0.5)
participant Provider as FidesUIProvider
end
rect rgba(255,240,200,0.5)
participant IconUtil as carbon-icon-defaults
participant AntModal as Ant Modal API
end
Consumer->>Provider: request modal methods from context
Provider->>Consumer: returns wrappedModalApi (info/success/warning/error/confirm)
Consumer->>Provider: call wrappedModalApi.info(args)
Provider->>IconUtil: getDefaultModalIcon(type="info")
IconUtil-->>Provider: ReactNode (icon)
Provider->>AntModal: call modalApi.info(args + icon)
AntModal-->>Provider: modal opened
Provider-->>Consumer: modal opened confirmation
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
gilluminate
added a commit
that referenced
this pull request
Mar 4, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR replaces Ant Design's default icons in the imperative Modal API ( Key points:
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: fe88a6d |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
clients/fidesui/src/components/feedback/ModalMethods.stories.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/components/feedback/ModalMethods.stories.tsx`:
- Line 5: The story imports PARAGRAPH_LOREM and TITLE_LOREM which contain a real
organization name; update ModalMethods.stories.tsx to remove that dependency and
replace usage of PARAGRAPH_LOREM and TITLE_LOREM with neutral placeholder copy
(either new constants like GENERIC_PARAGRAPH / GENERIC_TITLE defined locally in
the story or inline neutral strings such as "example_org" / generic lorem) so no
real customer/org names remain; ensure all references to PARAGRAPH_LOREM and
TITLE_LOREM in the ModalMethods story are replaced accordingly.
- Around line 20-26: The useEffect currently has an empty dependency array but
relies on modalApi, type, title, content, and hideIcon—update the effect to
include [modalApi, type, title, content, hideIcon] so Storybook arg changes
retrigger the modal; also implement cleanup by calling the modal instance's
destroy() (Modal.useModal() returns a ref with destroy) when the effect
unmounts/changes. Replace the imported PARAGRAPH_LOREM string that contains
"Ethyca" with a generic placeholder text (e.g., neutral lorem ipsum) so the
story no longer contains organization-specific content. Ensure references to
modalApi, type, title, content, hideIcon, PARAGRAPH_LOREM, and
Modal.useModal()/destroy() are updated accordingly.
In `@clients/fidesui/src/lib/carbon-icon-defaults.tsx`:
- Line 7: Import CarbonIconType from the package root instead of the internal
path: replace the import reference to "@carbon/icons-react/lib/CarbonIcon" with
the public export from "@carbon/icons-react" so the CarbonIconType symbol is
pulled from the package's public API (update the import that currently names
CarbonIconType to import it directly from "@carbon/icons-react").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19ba7959-ce68-4a24-820a-62f177f45b08
📒 Files selected for processing (4)
changelog/7569-modal-carbon-icons.yamlclients/fidesui/src/FidesUIProvider.tsxclients/fidesui/src/components/feedback/ModalMethods.stories.tsxclients/fidesui/src/lib/carbon-icon-defaults.tsx
clients/fidesui/src/components/feedback/ModalMethods.stories.tsx
Outdated
Show resolved
Hide resolved
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/lib/carbon-icon-defaults.tsx`:
- Line 8: The import is using Ant Design's internal path ("antd/lib") for the
Flex symbol; update the import to use Ant Design's public entrypoint instead by
replacing the current import of Flex (the Flex symbol) with an import from
"antd" so the module uses the documented public API rather than internal CJS
paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec38c06b-fcae-4a04-8dc9-fc7a2af9f3e4
📒 Files selected for processing (1)
clients/fidesui/src/lib/carbon-icon-defaults.tsx
8 tasks
speaker-ender
requested changes
Mar 6, 2026
Contributor
speaker-ender
left a comment
speaker-ender
left a comment
There was a problem hiding this comment.
Minor nits but otherwise looking great!
clients/fidesui/src/components/feedback/ModalMethods.stories.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+47
to
+56
| info: (props: Parameters<typeof modalApi.info>[0]) => | ||
| modalApi.info({ icon: getDefaultModalIcon("info"), ...props }), | ||
| success: (props: Parameters<typeof modalApi.success>[0]) => | ||
| modalApi.success({ icon: getDefaultModalIcon("success"), ...props }), | ||
| error: (props: Parameters<typeof modalApi.error>[0]) => | ||
| modalApi.error({ icon: getDefaultModalIcon("error"), ...props }), | ||
| warning: (props: Parameters<typeof modalApi.warning>[0]) => | ||
| modalApi.warning({ icon: getDefaultModalIcon("warning"), ...props }), | ||
| confirm: (props: Parameters<typeof modalApi.confirm>[0]) => | ||
| modalApi.confirm({ icon: getDefaultModalIcon("confirm"), ...props }), |
Contributor
There was a problem hiding this comment.
Not now but at some point might want to handle this with some kind of transform function since we probably need this type of pattern for notifications and messages at some point
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>
gilluminate
force-pushed
the
gill/ENG-2889/modal-carbon-icons
branch
from
cbbe49d to
2666242
Compare
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-2889
Description Of Changes
Replace Ant Design's default icons in the imperative Modal API (
modalApi.info(),.success(),.error(),.warning(),.confirm()) with Carbon icon equivalents from@carbon/icons-react, consistent with the rest of the FidesUI design system.The
useModalhook inFidesUIProvidernow wraps the modal API to inject Carbon icons as defaults. Callers can still override with a customiconprop or suppress withicon: null.Scope is limited to Modal methods only — message and notification APIs will follow in a future ticket.
Code Changes
clients/fidesui/src/lib/carbon-icon-defaults.tsx- New icon map module mapping modal types to Carbon icons (InformationFilled, CheckmarkFilled, WarningFilled, Misuse) with fidesui palette colorsclients/fidesui/src/FidesUIProvider.tsx- Wrap modalApi to inject default Carbon icons for each modal methodclients/fidesui/src/components/feedback/ModalMethods.stories.tsx- New Storybook stories for all 5 modal method types with hideIcon toggleSteps to Confirm
npm run storybookinclients/fidesui)icon: nullsuppresses the iconicon: nullcall sites (e.g., inuseRemoveCustomIntegration.tsx,PolicyConditionsTab.tsx) still suppress icons correctlyPre-Merge Checklist
CHANGELOG.mdupdatedSummary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Changelog