feat(system-setting): redesign page layout and information hierarchy#77
Conversation
.env holds the dev-only vite proxy target and must not be committed; only .env.example is tracked as the template.
Reduce visual noise on the System Settings page by pulling apart the three equal-weight layers (label / control / meta) into a clear hierarchy, and fix several layout issues. - Replace the ragged Row/Col grid with a CSS-column masonry so cards pack tightly without large vertical gaps. - Render each setting as a horizontal row: strong primary label, an inline DB/default source badge, and a de-emphasised monospace key, replacing the long redundant "effective value" sentence. - Use proper controls per type: a tri-state Switch for booleans (with a "reset to default" affordance that preserves the follow-default semantics) and a number stepper for integers. - Show the default value as an input placeholder when a field is unset, so non-bool defaults are visible. - Move the "Test email" action into the Email Service card header so it stays tied to the email config as settings grow. - Add per-category item counts, localized space/sidebar category titles, and a "saved at" hint in the toolbar. - Split the page into focused modules: helpers.ts (pure data helpers) and SettingRow.tsx (row rendering + custom controls).
lml2468
left a comment
There was a problem hiding this comment.
APPROVED ✅
UX 重设计方向合理,三态布尔语义保留,CI 全绿(build 18/20、CodeQL、osv-scan 全过),check-sprint 失败与本 PR 无关。
做得好的点
- 信息分级清晰:label(主)+ 控件(次)+ badge
DB/Default(来源)+ 等宽 key(末),替代了原来的扁平extra长串 BoolSwitch三态语义保留:'' = follow default/'1'/'0',显式覆盖时出现 "Reset to default" 链路回到 follow-default,没丢语义- 把 Test Email 从全局 toolbar 下沉到 Email Service 卡片 header,行为与配置归属一致
- 重构清晰:
helpers.ts(纯数据)+SettingRow.tsx(行渲染)+index.tsx(页面组合),职责分离干净 - 中英文 i18n 同步加齐(
category.space/category.sidebar/badge.*/savedAt/countItems),无漏键 .env加进.gitignore合理(dev-only vite proxy)
非阻塞 nit
- i18n 死键残留:
input.boolDefault/input.boolDefaultWithCurrent在 Select → Switch 重构后已无引用,可清理(en/zh 双侧);新增的input.followDefault*文案与之完全一致,复用其一更经济 - PR subtitle
"Login, registration policy, and email service configuration"没覆盖新加的 space / sidebar 分类,文案与实际范围对不上 - Test plan 里有 3 项 manual 未勾选(bool toggle round-trip / int placeholder / test-email flow)——这恰好是本 PR 的核心交互改动,合入前至少本地跑一遍验证 form value 在
''/'1'/'0'之间切换不丢 BoolSwitch在isDefault状态下,Switch 直接展示 effective 值,仅靠右侧 badge 区分 "DB / Default"——视觉上看不出"未显式配置"。可考虑给 default 态的 Switch 加灰色 className 强化区分(小 polish,不阻塞)
LGTM. 第 3 条 nit 建议作者合入前补一下手动 round-trip 验证;其余可后续 PR 清理。
Jerry-Xin
left a comment
There was a problem hiding this comment.
This PR is relevant to octo-admin and the System Setting page redesign builds successfully with no blocking regressions found.
💬 Non-blocking
🟡 Warning — src/pages/SystemSetting/SettingRow.tsx:23: The new boolean Switch makes the three-state model less explicit than the previous select. Users can still reach “explicitly configured to the same value as the current default” by toggling twice, but that is not discoverable. Consider keeping an explicit Default / Yes / No segmented control or adding clearer affordance for the default-vs-overridden state.
🔵 Suggestion — src/pages/SystemSetting/index.tsx:86: savedAt is a local HH:mm hint and is not cleared on manual refresh. This is minor, but after a later refresh it can still imply the page was just saved by the current user. Clearing it in fetchSettings when invoked manually would avoid stale feedback.
🔵 Suggestion — src/pages/SystemSetting/SettingRow.tsx:14, src/pages/SystemSetting/index.tsx:123, src/styles/admin.css:1093: The new comments are not in English. The repository already has mixed-language comments, so this is not a blocker, but OSS readability would improve if new implementation comments used English.
✅ Highlights
The helper extraction in src/pages/SystemSetting/helpers.ts preserves the existing encrypted-value keep behavior, including SECRET_MASK.
The new category grouping and per-card action placement in src/pages/SystemSetting/index.tsx is scoped cleanly and keeps the email test action tied to the support settings.
Verification: npm run build passed, and npm test passed. npm ci reported existing dependency audit findings, but they are not introduced by this PR’s changed files.
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #77 (octo-admin)
Verdict: APPROVE
A clean, well-structured redesign of the System Settings page. The refactor into helpers.ts (pure data) + SettingRow.tsx (rendering) + index.tsx (composition) is a genuine readability win, and the core data round-trip is preserved. I verified the PR's stated checks locally against head c4a662b:
npx tsc --noEmit→ passesnpm test→ 9/9 passnpm run build→ succeeds.gitignoreadds.envcorrectly;.envis not tracked and never appeared in history (.env.exampleremains the tracked template) — no secret leak.
The tri-state bool semantics ('' = follow default / '1' / '0') round-trip correctly through valuesFromSettings → BoolSwitch → valuesToPayload, and merely viewing the page never creates an override (Switch.onChange only fires on a real toggle). Encrypted keep-current via SECRET_MASK is preserved verbatim from the old code.
No P0/P1 issues found. The items below are all non-blocking suggestions (P2 — UX/maintainability/a11y) for a follow-up; none should block merge.
Suggestions (P2 — non-blocking)
1. Dead/duplicate i18n keys (recommend cleaning up)
Removing boolText / settingSource / genericSettingExtra / genericBoolDefaultLabel / renderSettingInput orphaned 9 keys in both en-US and zh-CN systemSetting.json (verified zero code references via grep):
bool.yes, bool.no, bool.unset, source.db, source.default, input.boolDefault, input.boolDefaultWithCurrent, extra.effectiveValue, extra.effectiveEncrypted.
Additionally, the newly-added input.followDefault / input.followDefaultWithCurrent are byte-for-byte duplicates of the now-orphaned input.boolDefault / input.boolDefaultWithCurrent. Pick one: either reuse the existing keys, or (better, since the placeholder now serves int/string too) keep followDefault* and delete the old pair. Shipping both risks translation drift.
2. Accessibility: labels not associated with controls
Moving from <Form.Item label={...}> (which antd renders as a real <label htmlFor> tied to the control id) to a plain <span class="setting-row-label"> + noStyle Form.Item (SettingRow.tsx:108, 50/58/75/87) drops the programmatic label↔control association. There's no htmlFor/id/aria-label/aria-labelledby, so screen readers announce the Input/InputNumber/Password/Switch with no accessible name, and clicking the label no longer focuses the field. The bare <Switch> (SettingRow.tsx:40) is the most affected.
Fix: give each control an id and render the label as <label htmlFor={id}>, or add aria-labelledby pointing at the label span; pass aria-label={settingLabel(item)} to the Switch.
3. int field allows non-integer input
<InputNumber controls .../> (SettingRow.tsx:76) has no precision/min/step, so a user can type 3.5 into an int-typed field and it saves as '3.5'. Consider precision={0} (and min where appropriate) so the control matches the declared int type.
4. "Follow default" bool state is visually indistinguishable from an explicit value
When value === '' (follow default), checked = effectiveOn (SettingRow.tsx:24-26), so an unconfigured-but-default-ON setting renders an identical lit Switch to an explicit ON. The only differentiator before interaction is the Default badge; the "Reset to default" link appears only after toggling. The old Select made the tri-state explicit via a "Follow default config (current: X)" option. Consider a muted/"following default" affordance on the switch itself so the distinction is legible at a glance.
5. Per-field effective-value/source context reduced
The old genericSettingExtra surfaced the effective value + source (DB vs default) under every field. The new badge conveys source, and int/string keep the effective value as a placeholder — but only when unconfigured (placeholder is undefined when configured), and encrypted never shows it. For already-configured non-bool fields the effective value still appears as the populated input, so this is minor, but a tooltip on the badge could restore the dropped context cheaply.
6. Masonry layout: column-major reading order + cross-browser fragmentation
.setting-masonry { column-count: 2 } (admin.css:~1093-1106) flows cards column-major (top-to-bottom in column 1, then column 2), so the visual scan order no longer matches source/category order (DOM/tab order is preserved). The Email Service card's position — which uniquely carries the Test Email action — becomes less predictable. Also, break-inside: avoid inside CSS multicol is honored inconsistently across engines for very tall cards; worth a quick Safari/Firefox check, optionally adding -webkit-column-break-inside: avoid; page-break-inside: avoid;. If row-major order matters, a CSS grid (grid-template-columns: 1fr 1fr) is an alternative, trading some vertical whitespace.
7. Encrypted clear-to-default not possible (pre-existing)
For a configured encrypted setting, blank = keep-current (emits SECRET_MASK), any text = new value — there's no way to clear an encrypted override back to follow-default (helpers.ts:40-46). This logic was moved verbatim from the old code (not a regression), but now that bools have an explicit "Reset to default" affordance the asymmetry is more noticeable. Out of scope for this PR.
Overall: solid refactor, semantics preserved, build/tests/typecheck green. Approving; the above are polish items for a follow-up.
Follow-up to the System Settings redesign, covering non-blocking review nits.
- Remove 9 orphaned i18n keys left after dropping the old renderers
(bool.*, source.*, input.boolDefault*, extra.*); keep the single
followDefault* pair now shared by int/string placeholders.
- Accessibility: give every control an aria-label so screen readers
announce a name (the noStyle Form.Item dropped the implicit label link).
- Mute the boolean Switch while it follows the default, so it reads
differently from an explicitly-set value at a glance.
- Constrain int inputs to whole numbers (precision={0}).
- Clear the "saved at" hint on a manual refresh so it never implies the
page was just saved; keep it across the post-save reload.
- Extend the page subtitle to mention the space/sidebar categories.
- Translate the new implementation comments to English; add
-webkit-/page-break-inside fallbacks for the masonry cards.
- Add helpers.test.ts covering the bool tri-state round-trip
(load -> toggle -> reset) and encrypted keep-current behavior.
|
Thanks for the thorough reviews 🙏 Pushed a follow-up commit (3b6f1ac) addressing the non-blocking nits:
Deferred to a later PR (out of scope here): label Verified locally: |
lml2468
left a comment
There was a problem hiding this comment.
APPROVED ✅
上轮所有 nit(含其他 reviewer 的)全部认真处理,且超出了"非阻塞"标签的要求强度,非常扎实:
| 上轮 nit | 本次处理 |
|---|---|
| i18n 死键残留 | 删除 9 个 orphaned key(bool.* / source.* / input.boolDefault* / extra.*),en/zh 双侧同步 |
| subtitle 文案不覆盖 space/sidebar | 改为 "Login, registration, space, sidebar, and email service" |
| BoolSwitch default 态视觉无差异 | 加 setting-switch-toggle--default className,opacity 0.55 / hover 0.8 |
| 注释非英文 | 全部翻译英文(SettingRow / index.tsx / admin.css) |
| savedAt 手动刷新不清除 | 加 keepSavedHint option,save 后保留、手动 refresh 清空 |
| 无障碍 label 关联 | 每个控件加 aria-label={settingLabel(item)}(noStyle Form.Item 丢失隐式 label 链接的合理修复) |
| Int 字段允许非整数 | InputNumber precision={0} |
| Test plan 未覆盖核心交互 | 新增 helpers.test.ts 116 行,覆盖 bool tri-state load → toggle → reset round-trip 及 encrypted SECRET_MASK keep-current 语义 |
额外亮点:
- CSS 加
-webkit-column-break-inside/page-break-insidefallback,masonry 兼容性更稳 - helpers.test.ts 显式验了一条关键约束:"Not configured stays empty (follow default) — viewing must not pre-fill from effective_value",把三态语义钉到测试里防回归
CI build/CodeQL pending 中(diff 内容已审),check-sprint 失败与本 PR 无关。
LGTM, no remaining nits.
Jerry-Xin
left a comment
There was a problem hiding this comment.
The PR is in scope and the System Settings redesign looks structurally sound; I found no blocking correctness, security, or architecture issues.
💬 Non-blocking
- 🟡 Warning: The
DB/Defaultbadge reflects only the last fetched persisted state, not the current unsaved form value. After a user edits a default-backed field or clears a DB-backed field, the badge can temporarily contradict the pending value until save/reload. Seesrc/pages/SystemSetting/SettingRow.tsx:116. - 🔵 Suggestion: The tri-state switch preserves all states, but setting an explicit value equal to the current default requires toggling away and then back. That is reachable, but less direct than the old select. Consider a small component-level test for this path around
src/pages/SystemSetting/SettingRow.tsx:25.
✅ Highlights
- Helper extraction is clean, and the serialization tests cover the important bool/encrypted round-trip cases.
- The encrypted value handling continues to avoid echoing the mask into the form.
- Moving category title mapping and payload conversion into
helpers.tskeepsindex.tsxfocused on page composition.
Verification note: I could not independently run npx tsc --noEmit or npm test in this checkout because dependencies are not installed (typescript / vitest unavailable in node_modules).
Summary
Reworks the System Settings page (
/system-setting) to cut visual noise and fix several layout problems. The core change: the page used to render label / control / meta at near-equal visual weight, producing a flat, noisy wall of text. This PR pulls those three layers into a clear hierarchy.Information hierarchy
Switchfor booleans and a number stepper for integers, so a control reads as a control — not another line of text.category.key. Currently effective: X (DB config)sentence is replaced by a compactDB/Defaultsource badge next to the label plus a faint monospace key line.Layout fixes
Row/Colgrid (cards paired per row → large blank gaps where heights differed) with a CSS-column masonry so cards pack tightly.Correctness
'' = follow default/1/0). TheSwitchshows the effective state; once toggled explicitly a "reset to default" link appears to clear back to follow-default, so no semantics are lost.Refactor
helpers.ts(pure data helpers) andSettingRow.tsx(row rendering + the customBoolSwitchcontrol), leavingindex.tsxas page composition..envto.gitignore(dev-only vite proxy target;.env.exampleremains the tracked template).Test plan
npx tsc --noEmitpassesnpm testpasses (9/9)npm run buildsucceeds🤖 Generated with Claude Code