[Prototype] Reuse UIEditorSpriteItem for sprite items#3198
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the sprite item components in the prototype UI, specifically updating SpriteItem.vue to utilize a more generic UIEditorSpriteItem.vue component. The prototype check script was also updated to reflect these structural changes. Feedback suggests using sprite.shortName for display consistency, restoring accessibility labels and tooltips for the hidden state indicator, and adding horizontal padding to the title row to match the previous implementation.
|
|
||
| <template> | ||
| <button class="prototype-sprite-item" :class="{ active }" type="button" @click="emit('select')"> | ||
| <UIEditorSpriteItem :name="sprite.name" :selected="active" :visible="!sprite.hidden" @click="emit('select')"> |
There was a problem hiding this comment.
The previous implementation used sprite.shortName for the display label. Switching to sprite.name might cause visual regressions if the full name is long or contains internal identifiers. Consider passing sprite.shortName to maintain consistency with the previous behavior, or adding a separate prop to UIEditorSpriteItem if you want to show the full name in the tooltip while keeping the short name for display.
<UIEditorSpriteItem :name="sprite.shortName" :selected="active" :visible="!sprite.hidden" @click="emit('select')">
| aria-hidden="true" | ||
| > | ||
| <path d="M3 3l14 14" /> | ||
| <path d="M8.6 8.6a2 2 0 0 0 2.8 2.8" /> | ||
| <path d="M6.1 6.6C4.5 7.5 3.2 8.7 2.1 10c2 2.6 4.6 4 7.9 4 1.1 0 2.1-.2 3-.5" /> | ||
| <path d="M10.8 6.1c3 .3 5.4 1.6 7.1 3.9-.6.8-1.3 1.5-2.1 2.1" /> | ||
| </svg> | ||
| v-html="eyeOffIcon" |
There was a problem hiding this comment.
The hidden state indicator has lost its accessibility label and tooltip compared to the original SpriteItem implementation. Adding aria-label and title ensures that users (including those using screen readers) understand what the icon represents.
aria-label="Invisible"
title="Invisible"
v-html="eyeOffIcon"
| width: 76px; | ||
| height: 22px; | ||
| align-items: center; | ||
| gap: 2px; |
There was a problem hiding this comment.
The title row is missing horizontal padding, which was present in both the previous SpriteItem and UIEditorSpriteItem implementations. Adding padding: 0 6px; will prevent the text from being flush against the edges of the item and maintain visual consistency with the rest of the UI.
gap: 2px;
padding: 0 6px;
There was a problem hiding this comment.
The refactor is structurally sound — the slot-based composition, centralised hidden-state icon, and parallel contract-check updates are all well-executed. A few issues worth addressing before merge.
Summary of findings: 1 functional regression (name vs shortName), 1 layout hazard (default slot in packed container), and several minor concerns around prop cleanup and contract-check robustness.
|
|
||
| <template> | ||
| <button class="prototype-sprite-item" :class="{ active }" type="button" @click="emit('select')"> | ||
| <UIEditorSpriteItem :name="sprite.name" :selected="active" :visible="!sprite.hidden" @click="emit('select')"> |
There was a problem hiding this comment.
Functional regression: sprite.shortName is silently dropped.
The SpriteItem prop type still declares shortName: string (used to be displayed in the title row), but now :name="sprite.name" passes the full canonical name instead. The SpriteCard mock data has both fields (e.g. shortName: '牛小花牛小花...' vs the longer full name). While UIEditorSpriteItem does CSS truncation, the pre-truncated shortName was the intended display value.
Either:
- Pass
:name="sprite.shortName"if short names are still the display intent, or - Remove
shortNamefrom thespriteprop type if CSS truncation is now the strategy.
As-is, shortName is declared but never forwarded, which will confuse future maintainers.
| v-html="eyeOffIcon" | ||
| ></span> | ||
| </div> | ||
| <slot></slot> |
There was a problem hiding this comment.
Default <slot> in a fully-packed fixed-height container.
UIBlockItem is 88×88px with flex-direction: column. The img slot (60px + 5px margin) plus the title row (22px) already fills 87px, leaving almost no room. This works today only because SpriteItem's corner <span> is position: absolute and escapes normal flow.
Any future consumer passing a non-absolutely-positioned element into the default slot will silently overflow the container. Consider adding a comment documenting this constraint, or adding overflow: hidden to UIBlockItem.
| visible?: boolean | ||
| }>() | ||
|
|
||
| const imgStyle: CSSProperties = { |
There was a problem hiding this comment.
imgStyle defined in <script setup> creates a new object per instance.
In Vue 3, <script setup> runs once per component instance, so each mounted sprite allocates its own imgStyle object. For a typical sprite list (tens of items) this is negligible, but hoisting it to a module-level const (outside <script setup>, or in a separate module) would make it a shared singleton.
Also, image sizing in an inline style object has higher specificity than any scoped CSS class, so consumers cannot override it without !important. Defining the dimensions as a scoped CSS class on a wrapper would be more idiomatic and overridable.
| <script setup lang="ts"> | ||
| import type { CSSProperties } from 'vue' | ||
| import eyeOffIcon from '@/assets/editor/ui-icons/eye-off.svg?raw' | ||
| import UIBlockItem from '@/components/editor/UIBlockItem.vue' |
There was a problem hiding this comment.
color prop removal is unguarded in the contract checks.
The color prop was removed, but check-prototype.mjs has no assertion confirming it's absent. If any existing caller still passes :color="...", Vue will emit a runtime warning but no compile error. A check like !prototypeUIEditorSpriteItem.includes('color?:') would close this gap alongside the other contract assertions.
| !prototypeSpriteItem.includes('<UIEditorSpriteItem') || | ||
| prototypeSpriteItem.includes('.prototype-sprite-item {') || | ||
| !prototypeUIEditorSpriteItem.includes("import UIBlockItem from '@/components/editor/UIBlockItem.vue'") || | ||
| !prototypeUIBlockItem.includes('width: 88px;') || |
There was a problem hiding this comment.
Fragile slot attribute-order dependency in contract check.
The check includes('<slot name="img" :style="imgStyle"') will silently pass if attributes are reordered (e.g., :style before name). Splitting into two separate assertions — one for includes('<slot name="img"') and one for includes('imgStyle') — would be more robust.
|
|
||
| <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="prototype-editor-sprite-item-title" :title="name"> |
There was a problem hiding this comment.
UIEditorSpriteItem 的样式实现方式也应该跟真实前端代码一致,比如
- 通过 UIBlockItemTitle 来做 title 样式的复用,并通过 slot
suffix来添加 eyeOff icon - 使用 tailwind CSS 而不是常规 classname + style
There was a problem hiding this comment.
已调整:新增 UIBlockItemTitle,UIEditorSpriteItem 现在通过它复用 title 样式,并用 #suffix slot 放 eyeOff icon;原来的常规 classname + scoped style 已移除,title / icon 样式改为 tailwind classes。
There was a problem hiding this comment.
另外现在 prototype 中的 editor 已经包含了 costume item 和 animation item 了,但是并没有去抽取对应的 CostumeItem 和 AnimationItem(与 SpriteItem 类似基于 UIEditorSpriteItem 实现),会导致后续对 UIEditorSprite 样式的改动不能体现在 costume item 和 animation item 上;这也是这个 PR 应当解决的
There was a problem hiding this comment.
已调整:抽出了 CostumeItem 和 AnimationItem,两者都基于 UIEditorSpriteItem 实现;editor 里的 costume / animation 列表也已改为使用这两个组件,所以后续 UIEditorSpriteItem 的样式变更会同步影响这三类 item。
There was a problem hiding this comment.
另外这里的绝大部分检查是没有意义的(像是一份跟实现细节高度耦合的单测)
如果要保留,建议也只保留一些原则性的检查,比如这些:
for (const file of sourceFiles) {
const text = readFileSync(file, 'utf8')
const rel = relative(root, file)
if (text.includes('spx-gui')) failures.push(`forbidden real frontend reference: ${rel}`)
if (/\baxios\b/.test(text)) failures.push(`forbidden server call primitive: ${rel}`)
if (/\bfetch\s*\(\s*['"`]https?:\/\//.test(text)) failures.push(`forbidden remote fetch call: ${rel}`)
if (text.includes('@scalar/api-reference')) failures.push(`forbidden docs runtime reference: ${rel}`)
}There was a problem hiding this comment.
这条我先收敛本 PR 新增的检查:不再校验具体 width/gap/icon color 这类样式细节,改成只校验结构原则:SpriteItem / CostumeItem / AnimationItem 都基于 UIEditorSpriteItem,且 UIEditorSpriteItem 通过 UIBlockItemTitle 复用 title 结构。check-prototype.mjs 里既有的大量历史细节检查范围比较大,单独清理更合适,不混进这个基础 PR。
There was a problem hiding this comment.
已继续调整:这次把 check-prototype.mjs 整体从 895 行实现细节断言缩到 25 行原则性扫描,只保留这几类 guard:禁止引用 spx-gui、禁止 axios、禁止远程 URL fetch、禁止 @scalar/api-reference。之前保留的大量历史细节检查已经删除。
There was a problem hiding this comment.
这里的几个 UIXxx 组件的路径跟真实的前端代码不同;对于这种在真实前端代码中有明确对应项的组件定义,尽量确保其路径、名字等与其在真实前端代码中的对应项一致,以方便后续的维护与对照
There was a problem hiding this comment.
已处理到 #3203:把 prototype 里的 UIBlockItem / UIBlockItemTitle / UIEditorSpriteItem 从 src/components/editor/ 移到 src/components/ui/block-items/,与真实前端的 spx-gui/src/components/ui/block-items/ 路径保持一致;SpriteItem / CostumeItem / AnimationItem 的引用也已同步更新。验证已跑路径断言、npm run test:prototype、npm run build,并刷新本地 preview 确认无 console error。
Issue
Follow-up foundation change requested from review on #3190.
Changes
SpriteItemto reuseUIEditorSpriteItem.UIEditorSpriteItem.Why
The sprite editor design PR needs this foundation first, because padding/gap/title changes should be applied through the shared editor item primitive instead of a hand-written sprite-only component.
Validation
npm run test:prototypenpm run build/editor/qingqing/niu-runwith no console errors