feat(ui): remove success icon - #1863
Conversation
Return `check circle` instead. See #1669 Signed-off-by: Franz Heidl <franz.heidl@sap.com>
🦋 Changeset detectedLatest commit: 686430e The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
|
There was a problem hiding this comment.
Pull request overview
This PR updates the ui-components Icon implementation to retire the success icon SVG and make success resolve to the checkCircle icon instead, including Storybook notes and a test update to validate the alias behavior.
Changes:
- Removed the
check_box.svg(success) SVG import and mappedKnownIconsEnum.successto rendercheck_circle.svg(CheckCircle). - Added explanatory comments in the Icon implementation and in Storybook for
success/checkCircleequivalence. - Updated/added tests around
successrendering behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/ui-components/src/components/Icon/Icon.component.tsx | Removes success SVG import; renders CheckCircle for success with explanatory comment. |
| packages/ui-components/src/components/Icon/Icon.test.tsx | Updates success expectations and adds a test for the alias behavior. |
| packages/ui-components/src/components/Icon/Icon.stories.ts | Adds Storybook notes documenting the success → checkCircle aliasing rationale. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (5)
packages/ui-components/src/components/Icon/Icon.stories.ts:164
- The comment uses "has been sundown" which is grammatically incorrect in this context; use a past participle like "has been sunset" to keep the note clear and professional.
// Note: `success` and `checkCircle` return the same icon. `success` (check_box.svg) has been sundown
// in favour of `checkCircle` to avoid visual inconsistency between two filled checkmark icons.
packages/ui-components/src/components/Icon/Icon.stories.ts:437
- The comment uses "has been sundown" which is grammatically incorrect in this context; use a past participle like "has been sunset" to keep the note clear and professional.
// Note: `success` and `checkCircle` return the same icon. `success` (check_box.svg) has been sundown
// in favour of `checkCircle` to avoid visual inconsistency between two filled checkmark icons.
packages/ui-components/src/components/Icon/Icon.component.tsx:815
- The comment uses "has been sundown" which is grammatically incorrect in this context; use a past participle like "has been sunset" to make the rationale easier to understand.
// `success` (check_box.svg) has been sundown in favour of `checkCircle` to avoid visual inconsistency.
// The `success` name is kept as a valid alias so existing callers continue to work.
packages/ui-components/src/components/Icon/Icon.test.tsx:280
- These two tests currently assert the same behavior (rendering
icon="success"results inalt="checkCircle"), so one of them is redundant and adds maintenance cost without increasing coverage. Consider consolidating into a single alias-focused test.
test("renders a success icon", () => {
render(<Icon icon="success" />)
expect(screen.getByRole("img")).toBeInTheDocument()
expect(screen.getByRole("img")).toHaveAttribute("alt", "checkCircle")
})
.changeset/witty-meals-know.md:5
- This changeset entry has a mismatched backtick ("
check circle´") which will break Markdown rendering, and the wording suggests thesuccessicon is removed entirely even though it remains as an alias. Update the text to reflect the alias behavior and use the correct icon name (checkCircle`).
feat(ui): remove `success` icon, return `check circle´ instead when `success` is called
keep alt „success“ when `success` was called, even though we technically return `check circle` Signed-off-by: Franz Heidl <franz.heidl@sap.com>
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
Signed-off-by: Franz Heidl <franz.heidl@sap.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/ui-components/src/components/Icon/Icon.test.tsx:287
- This test can produce a false positive: if either SVG has no (or the query selector changes), both
getAttribute("d")calls returnnulland the equality assertion still passes. Add explicit assertions that the path elements exist before comparing theirdattributes.
const { container: successContainer } = render(<Icon icon="success" />)
const { container: checkCircleContainer } = render(<Icon icon="checkCircle" />)
const successPath = successContainer.querySelector("svg path")?.getAttribute("d")
const checkCirclePath = checkCircleContainer.querySelector("svg path")?.getAttribute("d")
expect(successPath).toEqual(checkCirclePath)
.changeset/witty-meals-know.md:5
- The changeset message has a mismatched backtick and a stray accent in
check circle´. It should refer to the actual icon namecheckCircleand keep backticks balanced.
feat(ui): remove `success` icon, return `check circle´ instead when `success` is called
This PR removes the
successicon (for rationale see #1669), and returnscheck circleinstead when callingsuccess, and adds a test and necessary comments for the replacement.Summary
Changes Made
successiconIconcomponent to returncheck circlewhensuccessis calledRelated Issues
Closes #1669
Testing Instructions
pnpm ipnpm run test IconChecklist
PR Manifesto
Review the PR Manifesto for best practises.