Skip to content

[Demo][Component] Update: Implement Sprite panel spacing#3272

Draft
jiaxin-915 wants to merge 2 commits into
goplus:uifrom
jiaxin-915:issue-3247
Draft

[Demo][Component] Update: Implement Sprite panel spacing#3272
jiaxin-915 wants to merge 2 commits into
goplus:uifrom
jiaxin-915:issue-3247

Conversation

@jiaxin-915

Copy link
Copy Markdown
Contributor

Issue

PR Type

  • Design PR
  • Demo PR (Draft)
  • Docs PR

Changes

  • Applied the updated side padding demo for the Costumes list and Animations list in the Sprite panel.
  • Adjusted the left list column width from 120 to 112 to match the tighter spacing setup.
  • Updated the add button container width from 120 to 112 so the control area stays aligned with the list column.
  • Expanded the right preview/content area from 776 to 784 to preserve the overall panel balance after the spacing change.
  • Synced the demo layout between Costumes and Animations so both branches reflect the same spacing behavior.

Scope

In scope:

  • Demo implementation for Sprite panel Costumes and Animations list side padding adjustment.
  • Visual verification that the tighter spacing works in the current demo layout.

Out of scope:

  • Final production decision for all editor panels.
  • Broader spacing/token refactor outside the Sprite panel.
  • Any unrelated interaction or behavior changes.

Validation

  • Verified the updated demo layout in the current Sprite panel flow.
  • Confirmed the panel keeps visual balance after reallocating width from left to right.

Demo Purpose

  • Demonstrate how the Sprite panel reads after reducing side padding from 16px to 12px.

Related Design PR

Demo Entry

  • Screenshot:
issue-3247

Interaction Coverage

Validated:

  • Costumes list spacing update
  • Animations list spacing update
  • Left and right panel width rebalance
  • Add button alignment with the updated left column

Not validated:

  • Downstream implementation outside this demo scope

Demo-only Boundaries

  • This PR is only for demonstrating the spacing option in code.
  • The demo focuses on layout presentation and does not imply full production rollout.
  • Any follow-up token cleanup or broader panel standardization should be handled separately.

Production Handoff

  • Current demo reflects 12px side padding for both Costumes and Animations lists.
  • If this direction is approved, production implementation should keep the left list area, add button area, and right content area in the same width relationship used here.
  • Follow-up implementation can use this demo as the reference baseline for the Sprite panel spacing update.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request removes "peer": true from several dependencies in package-lock.json and updates the layout and styling of the EditorList component in spx-gui. Feedback from the reviewer highlights that increasing the horizontal padding of the items list to 12px can trigger a SortableJS bug when dragging items to the end of the list. It is recommended to keep the horizontal padding at 10px and use a small horizontal margin on the container to achieve the desired visual spacing.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

v-radar="listRadarInfo"
class="items mx-1.5 flex-[1_1_0] flex flex-col gap-2 overflow-y-auto"
>
<div ref="itemsWrapper" v-radar="listRadarInfo" class="items flex-[1_1_0] flex flex-col gap-2 overflow-y-auto">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Add mx-0.5 (2px horizontal margin) to the container to compensate for keeping the horizontal padding at 10px (to avoid SortableJS sorting bugs at the end of the list), achieving the desired 12px of total visual spacing.

      <div ref="itemsWrapper" v-radar="listRadarInfo" class="items mx-0.5 flex-[1_1_0] flex flex-col gap-2 overflow-y-auto">

/* Padding no more than `10px` disables the `_ghostIsLast` check. */
/* See details in https://github.com/SortableJS/Sortable/blob/ddd059717333d07b5b1125b7e1dc89514734bcf0/src/Sortable.js#L1822 */
padding: 12px 10px;
padding: 12px;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

By changing the horizontal padding of .items to 12px, you exceed the 10px limit that disables SortableJS's hardcoded _ghostIsLast spacer check (which is set to 10px). This can cause buggy behavior when dragging items to the very end of the list.\n\nTo achieve the desired 12px of visual spacing while keeping SortableJS happy, you can keep the horizontal padding at 10px and add a 2px horizontal margin (mx-0.5) to the container.

  padding: 12px 10px;

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

代码审查摘要

本 PR 为 Demo PR,聚焦于将 Sprite 面板列表的水平内边距从 16px 缩减至 12px。以下为主要发现:

spx-gui/package-lock.json — 批量移除 "peer": true 标记

多个包条目的 "peer": true 字段被移除,涉及 jwt-decodezodvitevuetypescript 等安全相关及核心包。package-lock.json 应仅由 npm 工具生成,手动修改或使用不同 npm 版本重新生成会导致依赖解析图不一致,可能在后续 npm ci 时静默安装与预期不同的包版本。建议确认该变更是通过标准 npm install 产生的,而非手动编辑,并在 CI 中加入 lockfile 一致性检查。

border-color: var(--ui-color-grey-400) !important;
background-color: var(--ui-color-grey-400) !important;
opacity: 0.6;
filter: grayscale(1);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[关键] 移除了 SortableJS 10px 水平内边距约束的说明注释

原注释记录了一个重要约束:SortableJS 使用 10 作为 spacer 阈值来决定是否触发 _ghostIsLast(将拖拽元素追加到列表末尾),因此水平 padding 须保持在 10px 以内。本 PR 删除了该注释,同时将水平 padding 从 10px 增大到 12px——恰好超过了注释中警告的临界值。

该注释的原始措辞在技术细节上可能并不完全精确(SortableJS 的 spacer 实际上与子元素的 bounding rect 比较,而非容器 padding 边缘),但它记录了一次真实的排查过程,并附有指向 SortableJS 源码的具体链接。删除注释的同时又做了注释所警告的那个改动,且未提供任何验证说明,会给后续维护者带来困惑。

建议:在合并前验证 12px 水平 padding 下拖拽到列表末尾的行为是否正常(尤其是 ghost 元素是否会意外跳到最后一位)。无论结果如何,请补充一条注释说明已验证该行为,或解释为何原有约束不再适用。

filter: grayscale(1);
box-shadow: var(--ui-box-shadow-sm);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[性能] filter: grayscale(1) 在拖拽过程中会触发昂贵的重绘

filter CSS 属性会强制浏览器将元素提升到独立合成层,并在每一帧对该元素及其所有子元素执行像素级颜色变换。在拖拽交互期间,ghost 元素每次移动都会触发一次完整的光栅化过程,在低性能设备或列表项包含图片缩略图(如 Sprite Costumes/Animations)时,可能导致拖拽卡帧。

相比之下,opacity: 0.6(本次变更已包含)在合成器层面处理,成本极低,能够实现相似的「淡化」视觉效果,而不需要 grayscale 滤镜带来的额外开销。

建议:考虑仅使用 opacity: 0.6 实现 ghost 元素的视觉降级效果,移除 filter: grayscale(1) 以避免拖拽帧率下降。如果保留灰度效果,请在目标设备上测试拖拽流畅度。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants