[Prototype] Fix sprite item title width#3203
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes several validation checks from the prototype script and updates the styling of the UIBlockItemTitle component to handle different sizes more explicitly. Additionally, a title attribute was added to the invisible icon in UIEditorSpriteItem for better accessibility. The reviewer suggested refactoring the UIBlockItemTitle component to move the common px-1.5 utility class to the base class string, which would reduce duplication and improve maintainability.
| 'flex items-center text-center text-title', | ||
| props.size === 'large' ? 'h-5 w-full gap-2 px-1.5 text-sm' : 'h-5.5 w-[76px] gap-0.5 px-1.5 text-2xs', |
There was a problem hiding this comment.
There was a problem hiding this comment.
The fix is correct and well-scoped: restoring w-[76px] for the medium title, removing the erroneous inline class override in UIEditorSpriteItem, and bringing back title="Invisible" are all right calls. A couple of things worth discussing:
Structural prototype checks were bundled with the bad CSS checks
The removed block in check-prototype.mjs contained two distinct types of checks:
- CSS dimension checks — these were enforcing the wrong layout (
w-full,px-1, now-[76px]) and are correctly removed. - Structural/architectural checks — these remain valid and were removed alongside the bad ones:
- The guard that
SpriteItem.vueimports and delegates toUIEditorSpriteItem(rather than duplicating its layout). - The guard that
UIBlockItemuses a<div>root and retains its 2px active pseudo-border.
- The guard that
The structural regression that PR #3198 was meant to prevent (SpriteItem duplicating UIEditorSpriteItem's layout) can now silently recur. Worth splitting these back out as separate checks.
Magic number with no derivation comment
w-[76px] appears once in the codebase with no explanation. The arithmetic is apparently: UIBlockItem 88px container − some margins/padding = 76px. A brief comment here would prevent the next silent regression when UIBlockItem is resized.
Minor: px-1.5 is identical in both ternary branches
It could be factored out as a shared unconditional class, making the two size variants easier to compare:
'flex items-center text-center text-title px-1.5',
props.size === 'large' ? 'h-5 w-full gap-2 text-sm' : 'h-5.5 w-[76px] gap-0.5 text-2xs',| } | ||
|
|
||
| for (const file of sourceFiles) { | ||
| const text = readFileSync(file, 'utf8') |
There was a problem hiding this comment.
The structural check that SpriteItem.vue imports and uses UIEditorSpriteItem (rather than duplicating its own hidden-icon layout) was a valid regression guard — the regression it prevented was precisely the one that necessitated PR #3198. It was removed alongside the bad CSS checks, but the two concerns are independent. Worth restoring just the structural checks here:
const prototypeSpriteItem = readFileSync(join(root, 'src/components/editor/SpriteItem.vue'), 'utf8')
if (
!prototypeSpriteItem.includes("import UIEditorSpriteItem from '@/components/editor/UIEditorSpriteItem.vue'") ||
!prototypeSpriteItem.includes('<UIEditorSpriteItem')
) {
failures.push('prototype SpriteItem must delegate to UIEditorSpriteItem instead of duplicating its layout')
}| 'flex w-full items-center gap-2 px-1.5 text-center text-title', | ||
| props.size === 'large' ? 'h-5 text-sm' : 'h-5.5 text-2xs', | ||
| 'flex items-center text-center text-title', | ||
| props.size === 'large' ? 'h-5 w-full gap-2 px-1.5 text-sm' : 'h-5.5 w-[76px] gap-0.5 px-1.5 text-2xs', |
There was a problem hiding this comment.
w-[76px] is an implicit coupling to UIBlockItem's internal dimensions. This is exactly the kind of silent dependency that caused the original regression. A comment explaining the derivation (e.g. /* UIBlockItem 88px − 2×border 1px − 2×padding 4px = 76px; adjust if UIBlockItem width changes */) would make it clear what to update if the container changes.
Also, px-1.5 is identical in both branches of the ternary and could be factored out as a shared unconditional class to make the size-variant differences easier to scan at a glance.
| 'flex items-center text-title', | ||
| props.size === 'large' | ||
| ? 'h-5 w-full gap-2 px-1.5 text-center text-sm' | ||
| : ['h-5.5 w-[76px] gap-0.5 px-0 text-2xs', slots.suffix != null ? 'text-left' : 'text-center'], |
There was a problem hiding this comment.
#3198 was merged with
UIBlockItemTitleusingw-fullandUIEditorSpriteItemoverriding the title row togap-0.5 px-1, which makes the sprite item title row wider than the intended 76px design.
w-[76px] 跟 w-full px-1 应该是等价的,因为 full 是 84px,减去两侧的 padding(4px * 1 * 2 = 8px)结果就是 76px;之前实现的版本我去看了下因为 #3203 (comment) 这里提到的原因所以实际宽度是不对的;不过现在没有这个问题了,所以应该还是 w-full + px-1 就行,要比 w-[76px] 稍好些
另外在有 suffix(右侧图标)的时候就把文本改成左对齐,这个是预期的改动吗?
There was a problem hiding this comment.
已改回用容器宽度和内边距表达,并撤掉按 suffix 改变文本对齐的逻辑。
Issue
#3198 was merged with
UIBlockItemTitleusingw-fullandUIEditorSpriteItemoverriding the title row togap-0.5 px-1, which makes the sprite item title row wider than the intended 76px design. The same commit also added prototype checks that preserved that incorrect implementation detail.Changes
UIBlockItemTitletitle row to fixedw-[76px]with the expected 2px suffix gap.UIEditorSpriteItemuse the shared medium title spec directly instead of overriding it with 4px padding.Validation
npm run test:prototypenpm run build/editor/qingqing/niu-runwith no console errors.