fix: align native chart interval and indicator popovers with design (OK-59326) - #12817
fix: align native chart interval and indicator popovers with design (OK-59326)#12817erikzou wants to merge 3 commits into
Conversation
…OK-59326) Interval labels were truncated in the extension sidebar: the pills used $bodyLgMedium while the design specifies $bodyMdMedium, and the dialog padded its content twice because Dialog's own Content already applies px/pb. Both are fixed, and the chips, section headers and popover paddings now follow the design tokens. The All intervals grid no longer surfaces resolutions the chart never reported. getAllIntervalOptions used to pad the list with synthesised disabled entries, which left the list and the edit page disagreeing; it is now getOrderedIntervalOptions and only sorts what the chart sent. Shared pill styling moves to NativeChartControlsShared so the interval and indicator grids cannot drift apart again.
| minimumFontScale={0.82} | ||
| color={getIndicatorTextColor({ isActive, isDisabled })} | ||
| > | ||
| <SizableText size="$bodyMdMedium" numberOfLines={1} color={textColor}> |
There was a problem hiding this comment.
[🟡 P2] 指标 pill 文字空间被压缩且同时移除了原生自动缩字，StochRSI 会被截断
问题
这一处同时发生了三个方向一致的收缩：共享的 NATIVE_CHART_OPTION_PILL_LAYOUT_PROPS 把指标 pill 的 px 从原来的 $2(8px) 提到 $2.5(10px)；IndicatorListPopoverContent 的容器内边距从 p="$3"(12px) 提到 p="$5"(20px)；同时这行把 adjustsFontSizeToFit / minimumFontScale={0.82} 删掉了，只留 numberOfLines={1}。
以 popover(width: 360，4 列，gap $2) 算：改前每格文字可用宽 (360-2*12)/4 - 2*8 - 2 = 60px，改后是 (360-2*20)/4 - 2*10 - 2 = 52px。而 APP_NATIVE_INDICATOR_OPTIONS 里最长的 StochRSI 在 $bodyMdMedium(14px, 500) 下约需 ~60px。同一个 IndicatorPill 也被移动端 IndicatorsDialogContent 复用，在 375pt 机型上可用宽约 55px。
影响
主图/副图指标网格里 StochRSI 这类长标签会被省略成 StochRS…。adjustsFontSizeToFit 是 iOS/Android 专有属性，之前正是它在原生指标弹窗里兜住了这个 case（可缩到 14*0.82≈11.5px），删除后原生侧从「能缩字显示完整」直接退化成「截断」，而这个 dialog 并不是本次设计对齐的目标界面。Web 侧原本就贴边（60px vs 60px），本次再少 8px 后必然溢出。
建议
要么给指标 pill 保留原生自动缩字，要么让 pill 的水平内边距按网格来源区分（intervals 从 $3→$2.5 是变宽，indicators 从 $2→$2.5 是变窄，不适合共用一个常量）：
// NativeChartControlsShared.ts
export const NATIVE_CHART_OPTION_PILL_LAYOUT_PROPS = {
// ...
px: '$2',
} as const;或在 IndicatorPill 上单独覆盖 px="$2" 并保留 adjustsFontSizeToFit / minimumFontScale，再用最长标签 StochRSI 在 375pt 机型和 360px popover 上各验一次。
Generated by Claude Code
| export function getOrderedIntervalOptions( | ||
| options: ITradingViewIntervalOption[], | ||
| ) { |
There was a problem hiding this comment.
Changed interval-list logic ships without any test coverage
Severity: non-severe
The rewritten interval-ordering logic (getOrderedIntervalOptions at packages/kit/src/components/TradingView/TradingViewV2/components/intervalSelector/NativeIntervalUtils.ts:152) changes which time resolutions the chart offers, yet no test file covers this module, so the new "only offer what the chart reported" behavior is unverified.
Impact: A future regression in which intervals appear (or wrongly disappear) from the chart menus would go unnoticed.
Repository rule: modified logic requires accompanying tests
.cursor/rules/self-testing.mdc Section 6 mandates that new/modified logic has a corresponding .test.ts file covering happy path and edge cases, and that a bug fix includes a regression test. The behavior change here is not pure styling: getAllIntervalOptions previously fabricated disabled: true placeholders for all 14 template resolutions, whereas getOrderedIntervalOptions (packages/kit/src/components/TradingView/TradingViewV2/components/intervalSelector/NativeIntervalUtils.ts:152-201) now drops unmatched templates entirely, which changes dialogOptions in packages/kit/src/components/TradingView/TradingViewV2/components/intervalSelector/hooks/useNativeIntervalSelector.ts:114-117 and therefore the edit grid, sorting order, and preferred-value reconciliation. There is no test file for NativeIntervalUtils.ts in the intervalSelector folder.
|
@codex review |
|
@codex security review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb9e0a8c6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| minimumFontScale={0.82} | ||
| color={getIndicatorTextColor({ isActive, isDisabled })} | ||
| > | ||
| <SizableText size="$bodyMdMedium" numberOfLines={1} color={textColor}> |
There was a problem hiding this comment.
在桌面端/扩展的 360px 指标弹窗中，p="$5" 和四列布局会让每个 pill 的实际文本宽度只剩约 52px；内置名称 StochRSI 在 14px $bodyMdMedium 下无法完整显示。这里同时移除 adjustsFontSizeToFit 和 minimumFontScale 后，它会被 numberOfLines={1} 截断，复现了本次改动原本要解决的标签省略问题。请为指标 pill 保留缩放能力，或为长指标名提供足够宽度。
Useful? React with 👍 / 👎.
| width: 360, | ||
| // Without this the focus scope moves focus to the Edit button on | ||
| // open and it renders with a focus ring the user never asked for. | ||
| onOpenAutoFocus: (event) => event.preventDefault(), |
There was a problem hiding this comment.
在 Web/扩展中用键盘激活 More 时，这里阻止默认的打开聚焦会让焦点留在弹窗外的触发器上；共享 Popover 又明确使用 trapFocus={false}，因此后续 Tab 会继续经过工具栏或页面背景，而不是直接进入弹窗中的 Edit 等操作，键盘用户无法正常按弹窗顺序操作。应通过 :focus-visible 等样式区分鼠标与键盘焦点，而不是取消弹窗的自动聚焦。
Useful? React with 👍 / 👎.
3 participants
OK-59326
Hot-update fix targeting
release/v6.5.0.Problem
Interval labels were truncated in the extension sidebar —
15分/30分/12时rendered as15…/30…/12….Two causes stacked:
$bodyLgMedium(16px); the design specifiesbodyMd-Medium(14px). TheadjustsFontSizeToFitfallback that compensated for this is iOS-only, so the overflow only surfaced on web/extension.Dialog's ownContentalready appliespx="$5" pb="$5", andIntervalsDialogContentadded anotherp="$5"on top — 40px of horizontal padding instead of 20px. In the narrow sidebar that left each chip ~62px, just under what a 16px CJK label needs.Changes
Typography and spacing, per design
$bodyLgMedium→$bodyMdMedium; removedadjustsFontSizeToFit/minimumFontScaleso every chip renders at one size$full→$2, px$3→$2.5, grid gap$2.5→$2$bgActive+$borderActive, otherwise$bgStrong; label$text(was$textSubduedwhen inactive)$bodyMd;Editentry → design-system Button (small / tertiary, hover background suppressed)contentContainerProps: { px: '$0', pb: '$0' }); popover and dialog now carry their own padding and group gapsBehaviour
All intervalsused the raw chart list while the edit page used a padded 14-item list, so the two disagreed.getAllIntervalOptionsfabricateddisabled: trueplaceholders for resolutions the chart never reported; it is nowgetOrderedIntervalOptionsand only orders what the chart actually sent. Nothing the chart cannot render is offered anywhere.Moretrigger (was 26px, which also squeezed the label)Editbutton an unrequested focus ringbottom-startlikeMore, so it expands to the right instead of off the chart's left edgeShared styling
NativeChartControlsSharedso the interval and indicator grids cannot drift apart againVerification
yarn agent:check --profile commitpasses; 9 TradingView test suites / 65 tests pass on this branch.Not visually verified in a running build — the extension side panel cannot be driven from the local preview tooling. Spacing and type figures above were derived from the design tokens and checked against
tamagui.config.ts.