[Design][Component] Feature: Add status circle buttons and sound playback button#3301
[Design][Component] Feature: Add status circle buttons and sound playback button#3301wenjiangping wants to merge 3 commits into
Conversation
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
There was a problem hiding this comment.
Code Review Summary
This PR adds sound playback buttons (Button/SoundPlay/36 and Button/SoundPlay/48) and circular status buttons (Button/Large/Danger/Circle/* and Button/Large/Success/Circle/*) to the component library. The overall structure is sound, but there are a few issues to address.
Findings:
-
Hard-coded hex color instead of design token — The
Stop/Defaultvariant insideButton/SoundPlay/36(and/48) uses raw hex#36C2CFfor the stop icon fill, while every other new component uses tokens like$turquoise500. This should be replaced with the appropriate token to stay consistent with the rest of the library and to survive future palette changes. -
Inconsistent
gapacross Danger/Circle states — Patch 3 removed"gap": 10fromButton/Large/Danger/Circle/Default, but the five sibling states (Hover, Click, Loading, Disabled, Focus) still carry"gap": 10. All states of a component variant should be structurally identical; either removegapfrom all states (if it serves no purpose for an icon-only circular button) or restore it on Default. -
Disabled placeholder layer in a published component — The frame
ccr79(icon/stop-filled) inside theStop/Defaultstate is marked"enabled": false. If this is a leftover debug/prototype layer it should be removed before merge. If it is intentional (e.g., a toggled-off alternative icon), add a note explaining the design intent so future maintainers don't treat it as dead weight. -
Missing Disabled/Focus states for sound-play buttons —
Button/SoundPlay/36andButton/SoundPlay/48cover only Default/Hover/Click, while the new circular status buttons cover all 6 states. If sound-play buttons can be disabled or focused, these states are missing. If they are intentionally omitted (e.g., the button is never disabled), that is fine — just confirm the intent. -
Sub-pixel coordinate drift noise — Several diff hunks change coordinates by floating-point rounding noise (e.g.,
14.999999999999993→14.99999999999999,5.682361148042482e-14→5.682361148043401e-14). These have no visual effect but inflate the diff and obscure meaningful changes. Consider snapping coordinates on export, or squash these into a separate cleanup commit so they don't hide real changes during review.
| "y": 2.6666667461395264, | ||
| "name": "stop", | ||
| "geometry": "M0 13l0-10c0-2 1-3 3-3l10 0c2 0 3 1 3 3l0 10c0 2-1 3-3 3l-10 0c-2 0-3-1-3-3z", | ||
| "fill": "#36C2CF", |
There was a problem hiding this comment.
Hard-coded hex #36C2CF here (and the same value appears in the 48px Stop/Default variant). All other fills in this PR use design tokens like $turquoise500. Replace with the appropriate token so the color stays in sync with palette changes.
| "x": 10, | ||
| "y": 10, | ||
| "name": "icon/stop- filled", | ||
| "enabled": false, |
There was a problem hiding this comment.
This "enabled": false child frame (icon/stop-filled) will be invisible in the rendered component. If it is a leftover prototype layer, remove it. If it is intentionally kept as a toggled-off variant, add a comment or note explaining why so future maintainers understand the intent.
| { | ||
| "type": "frame", | ||
| "id": "y3vG5", | ||
| "name": "Button/Large/Danger/Circle/Default", |
There was a problem hiding this comment.
Button/Large/Danger/Circle/Default has no gap after patch 3 removed it, but the Hover, Click, Loading, Disabled, and Focus siblings still carry "gap": 10. All states of a component variant should be structurally identical — either remove gap from all six states or restore it here.
Issue
PR Type
Changes
DangerandSuccesscircle button variants.Scope
In scope:
DangerandSuccesscircle buttons.Validation
Design Intent
Design Assets Changed
Production Handoff
DangerandSuccessas semantic variants instead of using hard-coded colors.