[Page] Update: Sprite editor design assets#3190
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates several x-coordinate values in ui/pages/spx/editor-sprite.pen to high-precision floating-point numbers. Feedback indicates that these values are likely artifacts from a design tool export and recommends reverting them to integer values to prevent potential rendering issues and maintain code cleanliness.
| "type": "ref", | ||
| "ref": "a:Hjqqt", | ||
| "x": 0, | ||
| "x": 0.011189733001048491, |
There was a problem hiding this comment.
The coordinate value 0.011189733001048491 appears to be a sub-pixel precision artifact, likely introduced by a design tool export. Such high-precision floating-point values for layout coordinates can lead to blurry rendering and unnecessary noise in the codebase. It is recommended to use integer values for coordinates unless sub-pixel positioning is intentionally required.
"x": 0,
| "type": "ref", | ||
| "ref": "a:Hjqqt", | ||
| "x": 0, | ||
| "x": 0.011189733001048491, |
There was a problem hiding this comment.
The coordinate value 0.011189733001048491 appears to be a sub-pixel precision artifact, likely introduced by a design tool export. Such high-precision floating-point values for layout coordinates can lead to blurry rendering and unnecessary noise in the codebase. It is recommended to use integer values for coordinates unless sub-pixel positioning is intentionally required.
"x": 0,
| "type": "ref", | ||
| "ref": "a:Hjqqt", | ||
| "x": 0, | ||
| "x": 0.011189733001048491, |
There was a problem hiding this comment.
The coordinate value 0.011189733001048491 appears to be a sub-pixel precision artifact, likely introduced by a design tool export. Such high-precision floating-point values for layout coordinates can lead to blurry rendering and unnecessary noise in the codebase. It is recommended to use integer values for coordinates unless sub-pixel positioning is intentionally required.
"x": 0,
There was a problem hiding this comment.
Code Review: Sprite Editor Design Assets
These are auto-generated design asset files (.pen / JSON format). Most numeric drift is expected from design-tool serialization. Two issues stand out as worth investigating before merge.
Summary
- 1 issue worth investigating (very large width values that may indicate a tool export bug)
- 1 structural inconsistency in frame naming/nesting across component variants
- 2 minor polish items (floating-point noise, missing EOF newline)
| "id": "Bjo4r", | ||
| "name": "Variant-Boring", | ||
| "width": 123849408531153920, | ||
| "width": 236177941850107520, |
There was a problem hiding this comment.
Suspicious width value — possible design-tool export bug
The "Variant-Boring" frame has "width": 236177941850107520 (~2.36 × 10^17 px). The sibling "Variant-Primary" at line 39372 has a similarly enormous value (260401320501400640). These values are above Number.MAX_SAFE_INTEGER, so JSON.parse will silently round them, meaning the value read at runtime will not match what is stored here — breaking any idempotency check or re-serialization.
All other explicit widths in this file are small integers or floats. Structurally equivalent variant-wrapper frames elsewhere in the file (e.g. lines ~45058 and ~45122) carry no explicit width field, suggesting that is the intended pattern for auto-layout variable-width frames.
Please verify whether this is a design-tool artifact (e.g., a "hug contents" or "fill container" frame whose natural width was erroneously serialized). If so, the width key should be removed from both frames.
| @@ -118086,7 +118086,7 @@ | |||
| { | |||
There was a problem hiding this comment.
Naming inconsistency across structurally paired variants
This outer frame is named "Frame 33" and contains a child "Frame 32" (two-level nesting for the label area). However, the sibling MediumMaxTextDefault variant at line 118249 uses a single-level "Frame 32" with no wrapper. This means the three MediumDefault* variants are not structurally consistent with each other.
The convention used in MediumDefault, MediumHover, and MediumActive is to name this container "Content". Consider either aligning the nesting structure across all three variants or renaming these auto-numbered frames to a descriptive name like "Content" to match the established pattern.
| "clip": true, | ||
| "width": 512, | ||
| "height": 828.0000000000002, | ||
| "height": 858.0000000000002, |
There was a problem hiding this comment.
Floating-point noise in height — 858.0000000000002 and 273.9999999999998 (line ~40085) are clearly intended to be 858 and 274. While visually imperceptible, these values will appear as false positives in any diff or equality check. If the design tool supports coordinate normalization/rounding on export, enabling it would reduce noise in future diffs.
| "a": "../../components/spx/builder-component.lib.pen" | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Missing newline at end of file. The closing } has no trailing newline, which is inconsistent with POSIX conventions and will annotate future diffs with \ No newline at end of file. If the design tool's export settings can be configured to always emit a trailing newline, that would prevent recurrence.
| .prototype-sprite-item-title { | ||
| display: flex; | ||
| width: 100%; | ||
| width: 76px; |
There was a problem hiding this comment.
这边目测是把原来 title 的边距从 6px 改成了 4px?建议还保持原来的实现方式:用宽度 100% 然后减去 6px 的 padding,而不是写死宽度 76px;前者能更好地反映得到 76 这个数字的逻辑
There was a problem hiding this comment.
已调整:title row 改回 width: 100% + padding: 0 6px,不再写死 76px。对应的 prototype contract check 也同步改成校验这个表达方式。
There was a problem hiding this comment.
改回 padding: 6px 吗?那样 title 的宽度就变成 72px 了,这边结果上是要 76px 还是 72px?
There was a problem hiding this comment.
结果上需要 76px。上一版 width: 100% + padding: 0 6px 在当前全局 box-sizing: border-box 下实际内容宽度会变成 72px,所以已改成 width: calc(100% - 8px):父级内容宽度 84px,左右各留 4px,title row 结果宽度为 76px。对应的 prototype contract check 也同步更新。
There was a problem hiding this comment.
那建议用原来的 width + padding(width: 100%; padding: 0 4px;)做法?这个做法会更接近真实代码的做法
|
|
||
| <div class="flex h-5.5 w-[76px] items-center gap-0.5 px-1.5 text-center text-2xs text-title"> | ||
| <span class="min-w-0 flex-1 overflow-hidden text-ellipsis whitespace-nowrap" :title="name"> | ||
| <div class="flex h-5.5 w-[76px] items-center gap-0.5 text-center text-2xs text-title" :title="name"> |
There was a problem hiding this comment.
我看在真实前端代码里最终的 SpriteItem 是基于 UIEditorSpriteItem 实现的,但是在现在的 prototype 这里上面的 SpriteItem 并没有基于这个 UIEditorSpriteItem 实现,这个不一致本身是个问题
把这一点做成跟真实代码一致(通过复用 UIEditorSpriteItem 来实现 SpriteItem)是有意义的,因为现在编辑器中不只是 sprite item,animation item 和 costume item 也是通过复用 UIEditorSpriteItem 来实现的,所以这边会有个决策是,上面那个对 padding、gap 的调整,是只影响 sprite item 还是也影响 animation item 和 costume item
There was a problem hiding this comment.
把 sprite item(& animation item、costume item)的实现改为跟真实前端代码一致——通过复用 UIEditorSpriteItem 来实现(另外 UIEditorSpriteItem 本身的实现也应该与真实前端代码一致),这个调整建议通过往 goplus/ui 提一个单独的 PR 来做,因为这个调整本身并不是这个 PR(或对应 issue)的意图,而是后者的基础
There was a problem hiding this comment.
这个结构一致性问题先不混到本 PR 里处理。我对照了 spx-gui/src/components/editor/sprite/SpriteItem.vue 和 spx-gui/src/components/ui/block-items/UIEditorSpriteItem.vue,prototype 里 sprite / animation / costume item 复用 UIEditorSpriteItem 的调整更适合按你后面说的拆到 goplus/ui 单独 PR;本 PR 只处理当前 sprite item 这次设计变更相关的 padding / gap 表达。
There was a problem hiding this comment.
prototype 里 sprite / animation / costume item 复用
UIEditorSpriteItem的调整更适合按你后面说的拆到goplus/ui单独 PR
这个需要先做,它会影响这个 PR 的做法:比如如果我们把 title 宽度从 76 改成 72 的话,我们需要明确到底是只有 sprite item 的 title 宽度调整,还是 animation item & costume item 的 title 宽度也调整
There was a problem hiding this comment.
已按这里的建议更新基础 PR #3198(commit e924e1d):SpriteItem 继续复用 UIEditorSpriteItem;UIBlockItem root 调整为和真实前端一致的 div;UIBlockItemTitle 保持 w-full;UIEditorSpriteItem 的 title row 改为 gap-0.5 px-1。也补了 contract check,覆盖 root div、active 2px 描边、复用 UIEditorSpriteItem、不再手写 title/hidden icon 布局、不再使用 w-[76px] / px-0。验证已跑 npm run test:prototype 和 npm run build。
b5962b2 to
9ca8480
Compare
Issue
Related to #3137
Background
Update the sprite editor design assets for the ongoing UI work.
Changes
Scope
ui/components/spx/builder-component.lib.penui/pages/spx/editor-sprite.penDesign System Impact