-
Notifications
You must be signed in to change notification settings - Fork 528
fix: align native chart interval and indicator popovers with design (OK-59326) #12817
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/v6.5.0
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,12 @@ import { | |
| import { ETranslations } from '@onekeyhq/shared/src/locale'; | ||
| import platformEnv from '@onekeyhq/shared/src/platformEnv'; | ||
|
|
||
| import { HEADER_ICON_BUTTON_STYLE_PROPS } from '../utils/NativeChartControlsShared'; | ||
| import { | ||
| HEADER_ICON_BUTTON_STYLE_PROPS, | ||
| NATIVE_CHART_OPTION_GRID_GAP, | ||
| NATIVE_CHART_OPTION_PILL_LAYOUT_PROPS, | ||
| getNativeChartOptionPillColors, | ||
| } from '../utils/NativeChartControlsShared'; | ||
|
|
||
| import { | ||
| canToggleTradingViewNativeIndicatorOn, | ||
|
|
@@ -33,14 +38,6 @@ import type { | |
| import type { GestureResponderEvent } from 'react-native'; | ||
|
|
||
| const INDICATOR_GRID_COLUMN_COUNT = 4; | ||
| const INDICATOR_GRID_ITEM_LAYOUT_PROPS = { | ||
| flex: 1, | ||
| flexBasis: 0, | ||
| h: 32, | ||
| minWidth: 0, | ||
| px: '$2', | ||
| borderWidth: 1, | ||
| } as const; | ||
| export const TRADING_VIEW_NATIVE_INDICATOR_QUICK_BAR_HEIGHT = 31; | ||
| const INDICATOR_QUICK_BAR_VERTICAL_PAN_THRESHOLD = 4; | ||
|
|
||
|
|
@@ -87,17 +84,19 @@ function IndicatorPill({ | |
| isDisabled: boolean; | ||
| onPress?: () => void; | ||
| }) { | ||
| const { color: textColor, ...pillColors } = getNativeChartOptionPillColors({ | ||
| isHighlighted: isActive, | ||
| isDisabled, | ||
| }); | ||
|
|
||
| return ( | ||
| <XStack | ||
| key={indicator.value} | ||
| testID={buildIndicatorItemTestID(indicator.value)} | ||
| {...INDICATOR_GRID_ITEM_LAYOUT_PROPS} | ||
| borderRadius="$full" | ||
| borderCurve="continuous" | ||
| borderColor={isActive ? '$bgReverse' : 'transparent'} | ||
| {...NATIVE_CHART_OPTION_PILL_LAYOUT_PROPS} | ||
| {...pillColors} | ||
| alignItems="center" | ||
| justifyContent="center" | ||
| bg="$bgStrong" | ||
| hoverStyle={{ | ||
| bg: '$bgStrongHover', | ||
| }} | ||
|
|
@@ -109,13 +108,7 @@ function IndicatorPill({ | |
| userSelect="none" | ||
| onPress={isDisabled ? undefined : onPress} | ||
| > | ||
| <SizableText | ||
| size="$bodyMdMedium" | ||
| numberOfLines={1} | ||
| adjustsFontSizeToFit | ||
| minimumFontScale={0.82} | ||
| color={getIndicatorTextColor({ isActive, isDisabled })} | ||
| > | ||
| <SizableText size="$bodyMdMedium" numberOfLines={1} color={textColor}> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| {indicator.label} | ||
| </SizableText> | ||
| </XStack> | ||
|
|
@@ -146,11 +139,14 @@ function IndicatorGrid({ | |
| }, [indicators]); | ||
|
|
||
| return ( | ||
| <YStack gap="$2"> | ||
| <YStack gap={NATIVE_CHART_OPTION_GRID_GAP}> | ||
| {rows.map((row, rowIndex) => { | ||
| const placeholderCount = INDICATOR_GRID_COLUMN_COUNT - row.length; | ||
| return ( | ||
| <XStack key={`indicator-row-${rowIndex}`} gap="$2"> | ||
| <XStack | ||
| key={`indicator-row-${rowIndex}`} | ||
| gap={NATIVE_CHART_OPTION_GRID_GAP} | ||
| > | ||
| {row.map((indicator) => { | ||
| const isDisabled = !canToggleTradingViewNativeIndicatorOn({ | ||
| indicatorValue: indicator.value, | ||
|
|
@@ -171,8 +167,8 @@ function IndicatorGrid({ | |
| {Array.from({ length: placeholderCount }).map((_, index) => ( | ||
| <Stack | ||
| key={`indicator-placeholder-${rowIndex}-${index}`} | ||
| {...INDICATOR_GRID_ITEM_LAYOUT_PROPS} | ||
| borderColor="transparent" | ||
| {...NATIVE_CHART_OPTION_PILL_LAYOUT_PROPS} | ||
| borderColor="$transparent" | ||
| opacity={0} | ||
| pointerEvents="none" | ||
| /> | ||
|
|
@@ -547,7 +543,7 @@ function IndicatorListPopoverContent({ | |
| ); | ||
|
|
||
| return ( | ||
| <YStack p="$3" gap="$5"> | ||
| <YStack p="$5" gap="$5"> | ||
| <IndicatorSection | ||
| title={intl.formatMessage({ | ||
| id: ETranslations.market_main_chart_indicators, | ||
|
|
@@ -595,7 +591,7 @@ export function IndicatorPopover({ | |
| }} | ||
| showHeader={false} | ||
| usingSheet={false} | ||
| placement="bottom-end" | ||
| placement="bottom-start" | ||
| floatingPanelProps={{ | ||
| width: 360, | ||
| }} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -127,6 +127,9 @@ export const TradingViewNativeIntervalSelector = memo( | |
| title: intl.formatMessage({ id: ETranslations.market_intervals }), | ||
| showFooter: false, | ||
| testID: 'trading-view-native-intervals-dialog', | ||
| // IntervalsDialogContent owns its own padding so the popover and the | ||
| // dialog share one layout; drop the default Dialog content padding. | ||
| contentContainerProps: { px: '$0', pb: '$0' }, | ||
| onClose: () => { | ||
| handleIntervalsDialogClose(dialogInstance); | ||
| }, | ||
|
|
@@ -174,6 +177,7 @@ export const TradingViewNativeIntervalSelector = memo( | |
| onIntervalChange={onIntervalChange} | ||
| onPreferredValuesChange={handlePreferredValuesChange} | ||
| onClose={closeIntervalsPopover} | ||
| mode={intervalControlMode} | ||
| maxPreferredIntervalCount={ | ||
| intervalControlMode === 'popover' | ||
| ? null | ||
|
|
@@ -202,6 +206,9 @@ export const TradingViewNativeIntervalSelector = memo( | |
| onOpenChange={handleIntervalsPopoverOpenChange} | ||
| floatingPanelProps={{ | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| }} | ||
| renderTrigger={ | ||
| <IntervalMoreTrigger | ||
|
|
@@ -249,11 +256,14 @@ export const TradingViewNativeIntervalSelector = memo( | |
| activeTextColor="$text" | ||
| inactiveTextColor="$textSubdued" | ||
| h={30} | ||
| p="$0.5" | ||
| // No frame padding: the item has to fill the full 30px so its | ||
| // active/hover background matches the adjacent More trigger. | ||
| p="$0" | ||
| segmentControlItemStyleProps={{ | ||
| minWidth: 42, | ||
| px: '$2.5', | ||
| py: '$1', | ||
| py: '$0', | ||
| justifyContent: 'center', | ||
| }} | ||
| /> | ||
| ) : null} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,14 +7,6 @@ import type { IntlShape } from 'react-intl'; | |
| export const MAX_VISIBLE_INTERVAL_COUNT = 4; | ||
| export const MAX_PREFERRED_INTERVAL_COUNT = 4; | ||
| export const INTERVAL_GRID_COLUMN_COUNT = 4; | ||
| export const INTERVAL_GRID_ITEM_LAYOUT_PROPS = { | ||
| flex: 1, | ||
| flexBasis: 0, | ||
| h: 32, | ||
| minWidth: 0, | ||
| px: '$3', | ||
| borderWidth: 1, | ||
| } as const; | ||
|
|
||
| const PREFERRED_INTERVAL_STORAGE_KEY = | ||
| 'trading_view_native_preferred_intervals_v1'; | ||
|
|
@@ -152,7 +144,14 @@ export function formatIntervalOptionDisplayLabel( | |
| return label; | ||
| } | ||
|
|
||
| export function getAllIntervalOptions(options: ITradingViewIntervalOption[]) { | ||
| /** | ||
| * Orders the intervals the chart reported by the canonical template order and | ||
| * appends anything it reported outside that list. The chart owns the interval | ||
| * list, so a resolution it never reported is never surfaced. | ||
| */ | ||
| export function getOrderedIntervalOptions( | ||
| options: ITradingViewIntervalOption[], | ||
| ) { | ||
|
Comment on lines
+152
to
+154
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed interval-list logic ships without any test coverage Severity: The rewritten interval-ordering logic ( Repository rule: modified logic requires accompanying tests
|
||
| const optionsByLabel = new Map<string, ITradingViewIntervalOption>(); | ||
| const optionsByValue = new Map<string, ITradingViewIntervalOption>(); | ||
| options.forEach((option) => { | ||
|
|
@@ -170,27 +169,23 @@ export function getAllIntervalOptions(options: ITradingViewIntervalOption[]) { | |
|
|
||
| const seenValues = new Set<string>(); | ||
| const seenLabels = new Set<string>(); | ||
| const allOptions: ITradingViewIntervalOption[] = | ||
| ALL_INTERVAL_OPTION_TEMPLATES.map((template) => { | ||
| const normalizedLabel = normalizeIntervalLabel(template.label); | ||
| const matchedOption = | ||
| optionsByLabel.get(normalizedLabel) ?? | ||
| optionsByValue.get(template.fallbackValue); | ||
| const option = matchedOption | ||
| ? { | ||
| ...matchedOption, | ||
| label: template.label, | ||
| } | ||
| : { | ||
| label: template.label, | ||
| value: template.fallbackValue, | ||
| disabled: true, | ||
| }; | ||
|
|
||
| seenLabels.add(normalizedLabel); | ||
| seenValues.add(option.value); | ||
| return option; | ||
| const allOptions: ITradingViewIntervalOption[] = []; | ||
| ALL_INTERVAL_OPTION_TEMPLATES.forEach((template) => { | ||
| const normalizedLabel = normalizeIntervalLabel(template.label); | ||
| const matchedOption = | ||
| optionsByLabel.get(normalizedLabel) ?? | ||
| optionsByValue.get(template.fallbackValue); | ||
| if (!matchedOption) { | ||
| return; | ||
| } | ||
|
|
||
| seenLabels.add(normalizedLabel); | ||
| seenValues.add(matchedOption.value); | ||
| allOptions.push({ | ||
| ...matchedOption, | ||
| label: template.label, | ||
| }); | ||
| }); | ||
|
|
||
| options.forEach((option) => { | ||
| const normalizedLabel = normalizeIntervalLabel(option.label); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[🟡 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是变窄,不适合共用一个常量):或在
IndicatorPill上单独覆盖px="$2"并保留adjustsFontSizeToFit/minimumFontScale,再用最长标签StochRSI在 375pt 机型和 360px popover 上各验一次。Claude session ↗
Generated by Claude Code