feat: use the shop c mark as the credits icon - #464
Conversation
Deploying ui2 with
|
| Latest commit: |
cc4fb20
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f73562ce.ui2-423.pages.dev |
| Branch Preview URL: | https://feat-credits-icon-shop-c.ui2-423.pages.dev |
decentraland-bot
left a comment
There was a problem hiding this comment.
Review — PR #464
Verdict: Approve ✅
Clean, well-scoped change. The old multi-layer gradient bloom is replaced with the hexagonal C badge that the shop already uses, and the dead NavbarCreditsIcon is removed. No P0 or P1 issues.
What I checked
-
Correctness of the new
CreditsIcon—viewBox="0 0 20 21"is placed before{...props}, following MUI convention (set a default, let the consumer override). The explicitfill="none"on the hexagon path is necessary because MUI'sSvgIconappliesfill: currentColorvia CSS on the root<svg>— without it the hexagon would render as a solid shape instead of a stroked outline. TheCletter path correctly usesfill="currentColor". Both paths match the existingShopCreditsIconartwork. -
Dead-code removal is safe —
NavbarCreditsIconwas defined and exported fromicons.tsxbut never imported byNavbar.tsx, never re-exported fromNavbar/index.ts, and never listed in the package barrel (src/index.ts). No downstream consumer in the Decentraland org imports it. Removal is a no-op for every consumer. -
No public API breakage —
CreditsIconkeeps the same signature (SvgIconProps). Internal consumers (Navbar.tsx,CreditsToggle.tsx,ManaBalances.tsx) passsx/fontSizebut neverviewBox, so the artwork swap is transparent. The marketplace'sCreditsIconis a separate image import, unrelated to this component. -
Security — No secrets, no XSS vectors (no
<script>, event handlers,<foreignObject>, or external references in the SVG), no unsafe React patterns. Clean.
P2 findings (non-blocking)
[P2] Orphaned JSDoc comment (src/components/Navbar/icons.tsx, line 248)
The comment /** Inline credits icon sized for the navbar (20×20). */ documented the now-deleted NavbarCreditsIcon. It currently floats above the unrelated chain-icon section comment, which reads oddly. Consider removing it.
Reviewed by Jarvis 🤖 · Requested by juanmahidalgo via GitHub
| @@ -246,42 +246,6 @@ const BellIcon = memo(function BellIcon(props: IconProps) { | |||
| }) | |||
|
|
|||
| /** Inline credits icon sized for the navbar (20x20). */ | |||
There was a problem hiding this comment.
[P2] This JSDoc documented the deleted NavbarCreditsIcon and is now orphaned — it sits above the chain-icon section comment. Consider removing it.
| /** Inline credits icon sized for the navbar (20x20). */ | |
| /** Chain icon components keyed by chainId. Uses the same SVGs as Icon/Chains/ but without MUI SvgIcon wrapper. */ |
|
Follow-up on the preview: the two C marks side by side came from the Dropped |
decentraland-bot
left a comment
There was a problem hiding this comment.
Review Summary
Clean, well-scoped change that replaces the old gradient bloom credits icon with the hexagonal C badge across all shared surfaces. The currentColor approach is a good design decision — it lets the glyph adapt to each surface's text colour rather than carrying a baked palette.
What I checked
- Public API surface:
CreditsIconis publicly exported via theIconbarrel, but only the rendered SVG changes — the component name andSvgIconPropsinterface are unchanged. Not a breaking change. - Dead code removal:
NavbarCreditsIconwas defined and exported inNavbar/icons.tsxbut never imported anywhere (Navbar.tsxusesShopCreditsIconand the MUI-wrappedCreditsIcon).Navbar/icons.tsxis not re-exported from the package entry point, so removing it has zero consumer impact. - Downstream consumers: The marketplace (
decentraland/marketplace) imports its own localCreditsIconSVG (../../images/icon-credits.svg), not the one from@dcl/ui2. No cross-repo breakage. - Internal consumers:
CreditsToggle,ManaBalances, andNavbarall importCreditsIconby name — the import paths and named export are unchanged, so all still work. - Story fix: Removing
creditsBalancefrom theShopWithManaInNavbarstory eliminates an impossible state (no app passes bothcreditsBalanceandshopCreditsBalance). - Security: No concerns — purely visual SVG changes, no user input, no auth, no secrets.
Findings
[P2] Orphaned JSDoc comment — icons.tsx line 248: The comment /** Inline credits icon sized for the navbar (20x20). */ documented the now-deleted NavbarCreditsIcon. It now sits orphaned above the chain icons section. Consider removing it. (Also noted in a previous inline comment.)
[P2] Duplicated SVG paths — The hexagonal C paths are duplicated between Icon/CreditsIcon (MUI SvgIcon wrapper) and Navbar/ShopCreditsIcon (plain <svg>). The JSDoc documents this as deliberate (MUI boundary) with the note "Change one and change the other." This is a reasonable trade-off — extracting shared path constants would add complexity for minimal gain. The warning comment is sufficient.
Verdict
No P0 or P1 issues. The two P2s are minor housekeeping items that don't block merge. Approved.
Reviewed by Jarvis 🤖 · Requested by juanmahidalgo via GitHub
| @@ -246,42 +246,6 @@ const BellIcon = memo(function BellIcon(props: IconProps) { | |||
| }) | |||
|
|
|||
| /** Inline credits icon sized for the navbar (20x20). */ | |||
There was a problem hiding this comment.
[P2] This JSDoc comment (/** Inline credits icon sized for the navbar (20x20). */) documented the now-deleted NavbarCreditsIcon and is orphaned — it sits directly above the chain-icon section comment. Consider removing it.
| /** Inline credits icon sized for the navbar (20x20). */ | |
| /** Chain icon components keyed by chainId. Uses the same SVGs as Icon/Chains/ but without MUI SvgIcon wrapper. */ |
The credits balance still rendered the old glyph — a multi-layer gradient bloom. The current mark for credits is the hexagonal C, which the shop already ships.
Changes
Icon/CreditsIconnow draws the hexagonal C, incurrentColor.NavbarCreditsIconfromNavbar/icons.tsx— the old bloom, dead code (not referenced anywhere, andNavbar/iconsis not exported from the package). Leaving it in would keep the retired glyph one import away.Because
CreditsIconis shared, this updates every surface where the glyph stands for the currency in one go: the navbar credits chip, the user menu balances, andCreditsToggle.Notes
currentColorinstead of a baked palette. The old mark carried its own gradients and so looked the same on every surface it landed on; the C inherits the text colour of whatever chip it sits in — lavender (#A0ABFF) in the navbar credits chip, off-white in the shop credits chip. That is what lets one glyph serve both without a per-surface variant.The notification illustrations are untouched.
Icon/Notifications/CreditsIconis a separate 48px artwork used by the credits notification rows, not a currency mark.The
creditsBalanceandshopCreditsBalancechips now share the glyph, distinguished by colour. No consumer passes both at once today (the marketplace passescreditsBalance, the shop passesshopCreditsBalance), and they are one currency, so this is intended rather than a collision to design around.Test plan
npm run format,npm run lint,npm run lint:package-json,npm run build,npm test(37 passing)Iconsstories) at default andfontSize="large"