From 3797f27f132bf541069a0cd2049fd0ba5ce6de5b Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Tue, 8 Sep 2026 17:42:24 +0200 Subject: [PATCH 01/15] feat: chart-shaped loading placeholder instead of spinner --- .../Charts/BarChart/BarChartContent.tsx | 11 +- src/components/Charts/BarChart/index.tsx | 4 + src/components/Charts/ChartSkeleton.tsx | 123 ++++++++++++++++++ .../Charts/LineChart/LineChartContent.tsx | 11 +- src/components/Charts/LineChart/index.tsx | 4 + .../Charts/PieChart/PieChartContent.tsx | 10 +- src/components/Charts/PieChart/index.tsx | 4 + src/components/Charts/SkiaWebChart/index.tsx | 10 +- src/styles/index.ts | 5 - .../components/Charts/ChartSkeleton.test.tsx | 48 +++++++ .../components/Charts/SkiaWebChart.test.tsx | 34 +++++ 11 files changed, 235 insertions(+), 29 deletions(-) create mode 100644 src/components/Charts/ChartSkeleton.tsx create mode 100644 tests/unit/components/Charts/ChartSkeleton.test.tsx diff --git a/src/components/Charts/BarChart/BarChartContent.tsx b/src/components/Charts/BarChart/BarChartContent.tsx index f89b8e00a2c2..84e922e07647 100644 --- a/src/components/Charts/BarChart/BarChartContent.tsx +++ b/src/components/Charts/BarChart/BarChartContent.tsx @@ -1,5 +1,5 @@ -import ActivityIndicator from '@components/ActivityIndicator'; import BAR_INNER_PADDING from '@components/Charts/barChartConstants'; +import ChartSkeleton from '@components/Charts/ChartSkeleton'; import ChartTooltipLayer from '@components/Charts/components/ChartTooltipLayer'; import ChartXAxisLabels from '@components/Charts/components/ChartXAxisLabels'; import ChartYAxisLabels from '@components/Charts/components/ChartYAxisLabels'; @@ -22,11 +22,12 @@ import useThemeStyles from '@hooks/useThemeStyles'; import variables from '@styles/variables'; +import CONST from '@src/CONST'; + import type {LayoutChangeEvent} from 'react-native'; import type {CartesianChartRenderArg, ChartBounds, PointsArray, Scale} from 'victory-native'; import React, {useState} from 'react'; -import {View} from 'react-native'; import {GestureDetector} from 'react-native-gesture-handler'; import Animated, {useAnimatedStyle, useSharedValue} from 'react-native-reanimated'; import {Bar, CartesianChart} from 'victory-native'; @@ -234,11 +235,7 @@ function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'l const chartPadding = {...VictoryTheme.axis.padding, bottom: labelSpace + VictoryTheme.axis.padding.bottom, left: yAxisLabelWidth + GLYPH_PADDING}; if (isLoading || !fontManager) { - return ( - - - - ); + return ; } if (data.length === 0) { diff --git a/src/components/Charts/BarChart/index.tsx b/src/components/Charts/BarChart/index.tsx index 1a7d477d6588..bfbf47f9b4f8 100644 --- a/src/components/Charts/BarChart/index.tsx +++ b/src/components/Charts/BarChart/index.tsx @@ -1,5 +1,8 @@ +import ChartSkeleton from '@components/Charts/ChartSkeleton'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; +import CONST from '@src/CONST'; + import React from 'react'; import type {BarChartProps} from './BarChartContent'; @@ -10,6 +13,7 @@ function BarChart(props: BarChartProps) { } /> ); } diff --git a/src/components/Charts/ChartSkeleton.tsx b/src/components/Charts/ChartSkeleton.tsx new file mode 100644 index 000000000000..2828af0464b7 --- /dev/null +++ b/src/components/Charts/ChartSkeleton.tsx @@ -0,0 +1,123 @@ +import type {ChartView} from '@components/Search/types'; +import SkeletonRect from '@components/SkeletonRect'; +import SkeletonViewContentLoader from '@components/SkeletonViewContentLoader'; + +import useContainerWidth from '@hooks/useContainerWidth'; +import useTheme from '@hooks/useTheme'; + +import CONST from '@src/CONST'; + +import type {ReactNode} from 'react'; + +import React from 'react'; +import {View} from 'react-native'; +import {Circle, Path} from 'react-native-svg'; + +import {CHART_CONTENT_MIN_HEIGHT} from './VictoryTheme'; + +const CHART_SKELETON_TEST_ID = 'chartSkeleton'; +const BAR_TEST_ID = 'chartSkeletonBar'; +const LINE_TEST_ID = 'chartSkeletonLine'; +const PIE_TEST_ID = 'chartSkeletonPie'; + +// Provisional shapes, distinguishable from each other by outline rather than by size. Design mocks replace them. +const BAR_HEIGHT_RATIOS = [0.45, 0.72, 0.34, 0.9, 0.56, 0.78]; +const BAR_GAP = 16; +const LINE_POINT_RATIOS = [0.72, 0.44, 0.6, 0.24, 0.38]; +const LINE_THICKNESS = 8; +const PIE_DIAMETER_RATIO = 0.72; + +function renderBarShape(width: number) { + const barWidth = Math.max((width - BAR_GAP * (BAR_HEIGHT_RATIOS.length - 1)) / BAR_HEIGHT_RATIOS.length, 0); + + return BAR_HEIGHT_RATIOS.map((ratio, index) => { + const barHeight = CHART_CONTENT_MIN_HEIGHT * ratio; + + return ( + + ); + }); +} + +function renderLineShape(width: number) { + const step = width / (LINE_POINT_RATIOS.length - 1); + const points = LINE_POINT_RATIOS.map((ratio, index) => ({x: index * step, y: CHART_CONTENT_MIN_HEIGHT * ratio})); + + // A stroked polyline would be dropped by the shimmer clip path, which only reads fill geometry, so the + // line is a filled band: the polyline out, then the same polyline back one thickness lower. + const topEdge = points.map((point, index) => `${index === 0 ? 'M' : 'L'}${point.x},${point.y}`).join(' '); + const bottomEdge = points + .slice() + .reverse() + .map((point) => `L${point.x},${point.y + LINE_THICKNESS}`) + .join(' '); + + return ( + + ); +} + +function renderPieShape(width: number) { + const radius = (Math.min(width, CHART_CONTENT_MIN_HEIGHT) * PIE_DIAMETER_RATIO) / 2; + + return ( + + ); +} + +const SHAPE_BY_VIEW: Record ReactNode> = { + [CONST.SEARCH.VIEW.BAR]: renderBarShape, + [CONST.SEARCH.VIEW.LINE]: renderLineShape, + [CONST.SEARCH.VIEW.PIE]: renderPieShape, +}; + +type ChartSkeletonProps = { + /** Chart view type whose placeholder shape to draw */ + view: ChartView; +}; + +/** + * The single place that maps a chart view type to the shape shown while that chart loads. Every shape draws into + * a box of the shared chart content height, so the three agree with the chart's own content box and with the card + * Home draws before the chart mounts. A per-type height would resize that card according to which type won. + */ +function ChartSkeleton({view}: ChartSkeletonProps) { + const theme = useTheme(); + const {onLayout, containerWidth} = useContainerWidth(); + + return ( + + + {SHAPE_BY_VIEW[view](containerWidth)} + + + ); +} + +export default ChartSkeleton; +export {BAR_TEST_ID, CHART_SKELETON_TEST_ID, LINE_TEST_ID, PIE_TEST_ID}; diff --git a/src/components/Charts/LineChart/LineChartContent.tsx b/src/components/Charts/LineChart/LineChartContent.tsx index 614aa459136b..db537a3ca1c1 100644 --- a/src/components/Charts/LineChart/LineChartContent.tsx +++ b/src/components/Charts/LineChart/LineChartContent.tsx @@ -1,4 +1,4 @@ -import ActivityIndicator from '@components/ActivityIndicator'; +import ChartSkeleton from '@components/Charts/ChartSkeleton'; import AreaGradient from '@components/Charts/components/AreaGradient'; import ChartTooltipLayer from '@components/Charts/components/ChartTooltipLayer'; import ChartXAxisLabels from '@components/Charts/components/ChartXAxisLabels'; @@ -24,11 +24,12 @@ import useThemeStyles from '@hooks/useThemeStyles'; import variables from '@styles/variables'; +import CONST from '@src/CONST'; + import type {LayoutChangeEvent} from 'react-native'; import type {CartesianChartRenderArg, ChartBounds, Scale} from 'victory-native'; import React, {useState} from 'react'; -import {View} from 'react-native'; import {GestureDetector} from 'react-native-gesture-handler'; import Animated, {useAnimatedStyle, useSharedValue} from 'react-native-reanimated'; import {CartesianChart, Line} from 'victory-native'; @@ -232,11 +233,7 @@ function LineChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = ' }; if (isLoading || !fontManager) { - return ( - - - - ); + return ; } if (data.length === 0) { diff --git a/src/components/Charts/LineChart/index.tsx b/src/components/Charts/LineChart/index.tsx index 7de73f7fcf44..f48b6912fb6e 100644 --- a/src/components/Charts/LineChart/index.tsx +++ b/src/components/Charts/LineChart/index.tsx @@ -1,5 +1,8 @@ +import ChartSkeleton from '@components/Charts/ChartSkeleton'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; +import CONST from '@src/CONST'; + import React from 'react'; import type {LineChartProps} from './LineChartContent'; @@ -10,6 +13,7 @@ function LineChart(props: LineChartProps) { } /> ); } diff --git a/src/components/Charts/PieChart/PieChartContent.tsx b/src/components/Charts/PieChart/PieChartContent.tsx index 33ea00f34383..bc2ec31be73d 100644 --- a/src/components/Charts/PieChart/PieChartContent.tsx +++ b/src/components/Charts/PieChart/PieChartContent.tsx @@ -1,4 +1,4 @@ -import ActivityIndicator from '@components/ActivityIndicator'; +import ChartSkeleton from '@components/Charts/ChartSkeleton'; import ChartTooltip from '@components/Charts/components/ChartTooltip'; import {TOOLTIP_BAR_GAP, useChartLabelFormats, useTooltipData} from '@components/Charts/hooks'; import type {ChartDataPoint, ChartProps, PieSlice, UnitPosition} from '@components/Charts/types'; @@ -9,6 +9,8 @@ import Text from '@components/Text'; import useLocalize from '@hooks/useLocalize'; import useThemeStyles from '@hooks/useThemeStyles'; +import CONST from '@src/CONST'; + import type {LayoutChangeEvent} from 'react-native'; import React, {useState} from 'react'; @@ -151,11 +153,7 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice }; if (isLoading) { - return ( - - - - ); + return ; } if (data.length === 0) { diff --git a/src/components/Charts/PieChart/index.tsx b/src/components/Charts/PieChart/index.tsx index 66999fee714f..d4f8503a4243 100644 --- a/src/components/Charts/PieChart/index.tsx +++ b/src/components/Charts/PieChart/index.tsx @@ -1,5 +1,8 @@ +import ChartSkeleton from '@components/Charts/ChartSkeleton'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; +import CONST from '@src/CONST'; + import React from 'react'; import type {PieChartProps} from './PieChartContent'; @@ -11,6 +14,7 @@ function PieChart(props: PieChartProps) { } /> ); } diff --git a/src/components/Charts/SkiaWebChart/index.tsx b/src/components/Charts/SkiaWebChart/index.tsx index 781d6f18354e..ede4457b6fec 100644 --- a/src/components/Charts/SkiaWebChart/index.tsx +++ b/src/components/Charts/SkiaWebChart/index.tsx @@ -10,7 +10,7 @@ import variables from '@styles/variables'; import viewRef from '@src/types/utils/viewRef'; -import type {ComponentType} from 'react'; +import type {ComponentType, ReactNode} from 'react'; import {WithSkiaWeb} from '@shopify/react-native-skia/lib/module/web'; import React, {useRef, useState} from 'react'; @@ -26,7 +26,9 @@ type SkiaWebChartProps = { /** Props forwarded to the lazily-loaded chart component. */ componentProps: TProps; - /** Identifies the loading skeleton span for telemetry. */ + /** Shown while the chart engine downloads. Callers that know their chart type pass its skeleton; the + * type-blind Victory renderer omits it and gets a spinner. */ + loadingFallback?: ReactNode; }; function ChartUnavailable() { @@ -55,7 +57,7 @@ function ChartUnavailable() { // `object` mirrors WithSkiaWeb's own constraint; `Record` would reject the // interface-based render-html renderer props (VictoryChartRendererProps) that lack an index signature. // eslint-disable-next-line @typescript-eslint/no-restricted-types -function SkiaWebChart({getComponent, componentProps}: SkiaWebChartProps) { +function SkiaWebChart({getComponent, componentProps, loadingFallback}: SkiaWebChartProps) { const styles = useThemeStyles(); const containerRef = useRef(null); @@ -72,7 +74,7 @@ function SkiaWebChart({getComponent, componentProps}: Ski return ; } - const fallback = ( + const fallback = loadingFallback ?? ( diff --git a/src/styles/index.ts b/src/styles/index.ts index 0c3c0ee8f549..7f29e66ed476 100644 --- a/src/styles/index.ts +++ b/src/styles/index.ts @@ -6631,11 +6631,6 @@ const staticStyles = (theme: ThemeColors) => chartContent: { height: CHART_CONTENT_MIN_HEIGHT, }, - chartActivityIndicator: { - height: CHART_CONTENT_MIN_HEIGHT, - justifyContent: 'center', - alignItems: 'center', - }, pieChartLegendContainer: { display: 'flex', justifyContent: 'center', diff --git a/tests/unit/components/Charts/ChartSkeleton.test.tsx b/tests/unit/components/Charts/ChartSkeleton.test.tsx new file mode 100644 index 000000000000..aad53217f9b4 --- /dev/null +++ b/tests/unit/components/Charts/ChartSkeleton.test.tsx @@ -0,0 +1,48 @@ +import {fireEvent, render, screen} from '@testing-library/react-native'; + +import ChartSkeleton, {BAR_TEST_ID, CHART_SKELETON_TEST_ID, LINE_TEST_ID, PIE_TEST_ID} from '@components/Charts/ChartSkeleton'; +import type {ChartView} from '@components/Search/types'; + +import CONST from '@src/CONST'; + +import React from 'react'; + +const CONTAINER_WIDTH = 320; + +// The shapes are all sized off the measured container, which never lays out under the test renderer, so every +// shape would otherwise draw at zero width and the assertions would pass on an empty box. +function renderAtContainerWidth(view: ChartView) { + render(); + fireEvent(screen.getByTestId(CHART_SKELETON_TEST_ID), 'layout', {nativeEvent: {layout: {width: CONTAINER_WIDTH, height: 0}}}); +} + +describe('ChartSkeleton', () => { + it('should draw a series of bars for the bar view', () => { + renderAtContainerWidth(CONST.SEARCH.VIEW.BAR); + + const bars = screen.getAllByTestId(BAR_TEST_ID); + + expect(bars.length).toBeGreaterThan(1); + expect(bars.every((bar) => Number(bar.props.width) > 0 && Number(bar.props.height) > 0)).toBe(true); + expect(screen.queryByTestId(LINE_TEST_ID)).toBeNull(); + expect(screen.queryByTestId(PIE_TEST_ID)).toBeNull(); + }); + + it('should draw a polyline band for the line view', () => { + renderAtContainerWidth(CONST.SEARCH.VIEW.LINE); + + // A closed band: `M` opens the top edge, then one `L` per remaining vertex along it and one per vertex + // back along the bottom edge. + expect(String(screen.getByTestId(LINE_TEST_ID).props.d)).toMatch(/^M0,[\d.]+( L[\d.]+,[\d.]+){9} Z$/); + expect(screen.queryByTestId(BAR_TEST_ID)).toBeNull(); + expect(screen.queryByTestId(PIE_TEST_ID)).toBeNull(); + }); + + it('should draw a disc for the pie view', () => { + renderAtContainerWidth(CONST.SEARCH.VIEW.PIE); + + expect(Number(screen.getByTestId(PIE_TEST_ID).props.r)).toBeGreaterThan(0); + expect(screen.queryByTestId(BAR_TEST_ID)).toBeNull(); + expect(screen.queryByTestId(LINE_TEST_ID)).toBeNull(); + }); +}); diff --git a/tests/unit/components/Charts/SkiaWebChart.test.tsx b/tests/unit/components/Charts/SkiaWebChart.test.tsx index 4b93cbb946c0..038fe5763b92 100644 --- a/tests/unit/components/Charts/SkiaWebChart.test.tsx +++ b/tests/unit/components/Charts/SkiaWebChart.test.tsx @@ -1,10 +1,13 @@ import {render, screen} from '@testing-library/react-native'; +import ActivityIndicator from '@components/ActivityIndicator'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; import isSkiaWebSupported from '@components/Charts/SkiaWebChart/isSkiaWebSupported'; +import Text from '@components/Text'; import {WithSkiaWeb} from '@shopify/react-native-skia/lib/module/web'; import React from 'react'; +import {View} from 'react-native'; jest.mock('@components/Charts/SkiaWebChart/isSkiaWebSupported', () => jest.fn()); @@ -59,4 +62,35 @@ describe('SkiaWebChart', () => { expect(mockWithSkiaWeb).toHaveBeenCalled(); }); + + it('should hand the caller-supplied loading fallback to Skia', () => { + mockIsSkiaWebSupported.mockReturnValue(true); + const loadingFallback = chart skeleton; + + render( + , + ); + + expect(mockWithSkiaWeb).toHaveBeenCalledWith(expect.objectContaining({fallback: loadingFallback}), undefined); + }); + + it('should keep its own spinner fallback when the caller supplies none', () => { + mockIsSkiaWebSupported.mockReturnValue(true); + + render( + , + ); + + const fallback = mockWithSkiaWeb.mock.lastCall?.at(0)?.fallback; + render({fallback}); + + expect(screen.UNSAFE_getByType(ActivityIndicator)).toBeTruthy(); + }); }); From 78ad6c252498a44294e9c9bc843f07058b1bf724 Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Tue, 8 Sep 2026 18:02:29 +0200 Subject: [PATCH 02/15] fix: scale chart skeleton bars to narrow containers --- src/components/Charts/ChartSkeleton.tsx | 21 +++++++++---------- src/components/Charts/SkiaWebChart/index.tsx | 3 +-- src/pages/home/HomePageSkeleton.tsx | 2 +- .../components/Charts/ChartSkeleton.test.tsx | 19 ++++++++++------- 4 files changed, 24 insertions(+), 21 deletions(-) diff --git a/src/components/Charts/ChartSkeleton.tsx b/src/components/Charts/ChartSkeleton.tsx index 2828af0464b7..b43bb8f56f40 100644 --- a/src/components/Charts/ChartSkeleton.tsx +++ b/src/components/Charts/ChartSkeleton.tsx @@ -20,27 +20,26 @@ const BAR_TEST_ID = 'chartSkeletonBar'; const LINE_TEST_ID = 'chartSkeletonLine'; const PIE_TEST_ID = 'chartSkeletonPie'; -// Provisional shapes, distinguishable from each other by outline rather than by size. Design mocks replace them. +// Provisional shapes. Design mocks replace them. const BAR_HEIGHT_RATIOS = [0.45, 0.72, 0.34, 0.9, 0.56, 0.78]; -const BAR_GAP = 16; +const BAR_GAP_RATIO = 0.25; const LINE_POINT_RATIOS = [0.72, 0.44, 0.6, 0.24, 0.38]; const LINE_THICKNESS = 8; const PIE_DIAMETER_RATIO = 0.72; function renderBarShape(width: number) { - const barWidth = Math.max((width - BAR_GAP * (BAR_HEIGHT_RATIOS.length - 1)) / BAR_HEIGHT_RATIOS.length, 0); + const slotWidth = width / BAR_HEIGHT_RATIOS.length; return BAR_HEIGHT_RATIOS.map((ratio, index) => { const barHeight = CHART_CONTENT_MIN_HEIGHT * ratio; return ( ); @@ -51,8 +50,8 @@ function renderLineShape(width: number) { const step = width / (LINE_POINT_RATIOS.length - 1); const points = LINE_POINT_RATIOS.map((ratio, index) => ({x: index * step, y: CHART_CONTENT_MIN_HEIGHT * ratio})); - // A stroked polyline would be dropped by the shimmer clip path, which only reads fill geometry, so the - // line is a filled band: the polyline out, then the same polyline back one thickness lower. + // A stroked polyline would be dropped by the shimmer clip path, which reads fill geometry only, so the + // line is drawn as a filled band. const topEdge = points.map((point, index) => `${index === 0 ? 'M' : 'L'}${point.x},${point.y}`).join(' '); const bottomEdge = points .slice() @@ -93,9 +92,9 @@ type ChartSkeletonProps = { }; /** - * The single place that maps a chart view type to the shape shown while that chart loads. Every shape draws into - * a box of the shared chart content height, so the three agree with the chart's own content box and with the card - * Home draws before the chart mounts. A per-type height would resize that card according to which type won. + * Every shape draws into a box of the shared chart content height, so the three agree with the chart's own + * content box and with the card Home draws before the chart mounts. A per-type height would resize that card + * according to which type won. */ function ChartSkeleton({view}: ChartSkeletonProps) { const theme = useTheme(); diff --git a/src/components/Charts/SkiaWebChart/index.tsx b/src/components/Charts/SkiaWebChart/index.tsx index ede4457b6fec..ec06f62bffdd 100644 --- a/src/components/Charts/SkiaWebChart/index.tsx +++ b/src/components/Charts/SkiaWebChart/index.tsx @@ -26,8 +26,7 @@ type SkiaWebChartProps = { /** Props forwarded to the lazily-loaded chart component. */ componentProps: TProps; - /** Shown while the chart engine downloads. Callers that know their chart type pass its skeleton; the - * type-blind Victory renderer omits it and gets a spinner. */ + /** Shown while the chart engine downloads. */ loadingFallback?: ReactNode; }; diff --git a/src/pages/home/HomePageSkeleton.tsx b/src/pages/home/HomePageSkeleton.tsx index 61fb06405281..da3a4a4450a1 100644 --- a/src/pages/home/HomePageSkeleton.tsx +++ b/src/pages/home/HomePageSkeleton.tsx @@ -29,7 +29,7 @@ const TRAILING_SUB_BAR_WIDTH = 40; const CARD_TITLE_WIDTH = 120; const LOWER_BAR_WIDTH = 80; -// The chart this stands in for holds its own loading spinner at exactly this height. +// The chart this stands in for draws its own loading skeleton at exactly this height. const SPINNER_CARD_HEIGHT = CHART_CONTENT_MIN_HEIGHT; const ROWS_PER_LIST_CARD = 3; const ROWS_PER_TABLE_CARD = CONST.HOME.SECTION_VISIBLE_LIMIT; diff --git a/tests/unit/components/Charts/ChartSkeleton.test.tsx b/tests/unit/components/Charts/ChartSkeleton.test.tsx index aad53217f9b4..867f8b01c002 100644 --- a/tests/unit/components/Charts/ChartSkeleton.test.tsx +++ b/tests/unit/components/Charts/ChartSkeleton.test.tsx @@ -8,12 +8,13 @@ import CONST from '@src/CONST'; import React from 'react'; const CONTAINER_WIDTH = 320; +const NARROW_CONTAINER_WIDTH = 60; -// The shapes are all sized off the measured container, which never lays out under the test renderer, so every -// shape would otherwise draw at zero width and the assertions would pass on an empty box. -function renderAtContainerWidth(view: ChartView) { +// The shapes size off the measured container, which never lays out under the test renderer, so without this +// every shape would draw at zero width and the assertions would pass on an empty box. +function renderAtContainerWidth(view: ChartView, width = CONTAINER_WIDTH) { render(); - fireEvent(screen.getByTestId(CHART_SKELETON_TEST_ID), 'layout', {nativeEvent: {layout: {width: CONTAINER_WIDTH, height: 0}}}); + fireEvent(screen.getByTestId(CHART_SKELETON_TEST_ID), 'layout', {nativeEvent: {layout: {width, height: 0}}}); } describe('ChartSkeleton', () => { @@ -28,12 +29,16 @@ describe('ChartSkeleton', () => { expect(screen.queryByTestId(PIE_TEST_ID)).toBeNull(); }); + it('should keep every bar drawn in a container narrower than the bar series', () => { + renderAtContainerWidth(CONST.SEARCH.VIEW.BAR, NARROW_CONTAINER_WIDTH); + + expect(screen.getAllByTestId(BAR_TEST_ID).every((bar) => Number(bar.props.width) > 0)).toBe(true); + }); + it('should draw a polyline band for the line view', () => { renderAtContainerWidth(CONST.SEARCH.VIEW.LINE); - // A closed band: `M` opens the top edge, then one `L` per remaining vertex along it and one per vertex - // back along the bottom edge. - expect(String(screen.getByTestId(LINE_TEST_ID).props.d)).toMatch(/^M0,[\d.]+( L[\d.]+,[\d.]+){9} Z$/); + expect(String(screen.getByTestId(LINE_TEST_ID).props.d)).toMatch(/^M0,[\d.]+( L[\d.]+,[\d.]+)+ Z$/); expect(screen.queryByTestId(BAR_TEST_ID)).toBeNull(); expect(screen.queryByTestId(PIE_TEST_ID)).toBeNull(); }); From db16116816470533d0a18dc7e790f76a0dd65e09 Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Tue, 8 Sep 2026 18:41:02 +0200 Subject: [PATCH 03/15] feat: draw chart skeleton in the home skeleton wall card --- src/components/Charts/ChartSkeleton.tsx | 8 +- src/components/Charts/SkiaWebChart/index.tsx | 9 +- src/pages/home/HomePage.tsx | 6 +- src/pages/home/HomePageSkeleton.tsx | 96 +++++++++---------- .../components/Charts/ChartSkeleton.test.tsx | 3 +- tests/unit/pages/HomePage.test.tsx | 10 +- .../unit/pages/home/HomePageSkeletonTest.tsx | 17 ++-- 7 files changed, 65 insertions(+), 84 deletions(-) diff --git a/src/components/Charts/ChartSkeleton.tsx b/src/components/Charts/ChartSkeleton.tsx index b43bb8f56f40..680ffd96347e 100644 --- a/src/components/Charts/ChartSkeleton.tsx +++ b/src/components/Charts/ChartSkeleton.tsx @@ -50,8 +50,7 @@ function renderLineShape(width: number) { const step = width / (LINE_POINT_RATIOS.length - 1); const points = LINE_POINT_RATIOS.map((ratio, index) => ({x: index * step, y: CHART_CONTENT_MIN_HEIGHT * ratio})); - // A stroked polyline would be dropped by the shimmer clip path, which reads fill geometry only, so the - // line is drawn as a filled band. + // A stroked polyline would be dropped by the shimmer clip path, which reads fill geometry only. const topEdge = points.map((point, index) => `${index === 0 ? 'M' : 'L'}${point.x},${point.y}`).join(' '); const bottomEdge = points .slice() @@ -91,11 +90,6 @@ type ChartSkeletonProps = { view: ChartView; }; -/** - * Every shape draws into a box of the shared chart content height, so the three agree with the chart's own - * content box and with the card Home draws before the chart mounts. A per-type height would resize that card - * according to which type won. - */ function ChartSkeleton({view}: ChartSkeletonProps) { const theme = useTheme(); const {onLayout, containerWidth} = useContainerWidth(); diff --git a/src/components/Charts/SkiaWebChart/index.tsx b/src/components/Charts/SkiaWebChart/index.tsx index ec06f62bffdd..487a3ba5b726 100644 --- a/src/components/Charts/SkiaWebChart/index.tsx +++ b/src/components/Charts/SkiaWebChart/index.tsx @@ -49,9 +49,8 @@ function ChartUnavailable() { } /** - * Shared web wrapper around `WithSkiaWeb` for the chart entry points (Pie/Line/Bar and the Victory - * renderer). When the environment can't provide a usable WebGL/Skia surface it shows an "unable to - * display chart" empty state instead of mounting Skia, avoiding the CanvasKit GL-init crash (see `isSkiaWebSupported`). + * When the environment can't provide a usable WebGL/Skia surface this shows an "unable to display chart" + * empty state instead of mounting Skia, avoiding the CanvasKit GL-init crash (see `isSkiaWebSupported`). */ // `object` mirrors WithSkiaWeb's own constraint; `Record` would reject the // interface-based render-html renderer props (VictoryChartRendererProps) that lack an index signature. @@ -64,11 +63,9 @@ function SkiaWebChart({getComponent, componentProps, load // while a fresh chart still re-checks capability instead of trusting a stale session-wide result. const [isSupported] = useState(() => isSkiaWebSupported()); - // The probe can pass while the renderer still ends up without a drawing surface, so also listen for the - // renderer reporting that and degrade to the empty state. + // The probe can pass while the renderer still ends up without a drawing surface. const isSurfaceUnavailable = useIsSkiaSurfaceUnavailable(containerRef); - // If unsupported, the device can't give CanvasKit a usable WebGL surface. if (!isSupported || isSurfaceUnavailable) { return ; } diff --git a/src/pages/home/HomePage.tsx b/src/pages/home/HomePage.tsx index 87fb1a03df24..66346fff3ed6 100644 --- a/src/pages/home/HomePage.tsx +++ b/src/pages/home/HomePage.tsx @@ -25,7 +25,7 @@ import DiscoverSection from './DiscoverSection'; import ForYouSection from './ForYouSection'; import FreeTrialSection from './FreeTrialSection'; import GettingStartedSection from './GettingStartedSection'; -import {HomePageSkeletonRowCards, HomePageSkeletonSpinnerCard} from './HomePageSkeleton'; +import {HomePageSkeletonChartCard, HomePageSkeletonRowCards} from './HomePageSkeleton'; import InsightsSection from './InsightsSection'; import RecentlyAddedSection from './RecentlyAddedSection'; import UpcomingTravelSection from './UpcomingTravelSection'; @@ -70,7 +70,7 @@ function HomePage() { {forYouSection} {shouldShowHomeSkeleton ? ( <> - + ) : ( @@ -92,7 +92,7 @@ function HomePage() { > {forYouSection} {shouldShowHomeSkeleton ? ( - + ) : ( <> diff --git a/src/pages/home/HomePageSkeleton.tsx b/src/pages/home/HomePageSkeleton.tsx index da3a4a4450a1..afec3019db25 100644 --- a/src/pages/home/HomePageSkeleton.tsx +++ b/src/pages/home/HomePageSkeleton.tsx @@ -1,5 +1,4 @@ -import ActivityIndicator from '@components/ActivityIndicator'; -import {CHART_CONTENT_MIN_HEIGHT} from '@components/Charts/VictoryTheme'; +import ChartSkeleton from '@components/Charts/ChartSkeleton'; import SkeletonRect from '@components/SkeletonRect'; import ItemListSkeletonView from '@components/Skeletons/ItemListSkeletonView'; import SkeletonTextLine, {BAR_HEIGHT} from '@components/Skeletons/SkeletonTextLine'; @@ -7,7 +6,6 @@ import WidgetContainer from '@components/WidgetContainer'; import useContainerWidth from '@hooks/useContainerWidth'; import useResponsiveLayout from '@hooks/useResponsiveLayout'; -import useStyleUtils from '@hooks/useStyleUtils'; import useThemeStyles from '@hooks/useThemeStyles'; import {lineHeightScale} from '@styles/typography'; @@ -29,13 +27,10 @@ const TRAILING_SUB_BAR_WIDTH = 40; const CARD_TITLE_WIDTH = 120; const LOWER_BAR_WIDTH = 80; -// The chart this stands in for draws its own loading skeleton at exactly this height. -const SPINNER_CARD_HEIGHT = CHART_CONTENT_MIN_HEIGHT; const ROWS_PER_LIST_CARD = 3; const ROWS_PER_TABLE_CARD = CONST.HOME.SECTION_VISIBLE_LIMIT; const CARD_TEST_ID = 'homePageSkeletonCard'; -const SPINNER_TEST_ID = 'homePageSkeletonSpinner'; // Two widths alternating down the card, so stacked rows read as separate rows rather than one block. const TWO_BAR_ROW_BAR_WIDTHS = [140, 110] as const; @@ -107,22 +102,14 @@ function renderIconTwoBarWithTrailingRow(args: SkeletonRowArgs) { ); } -type HomePageSkeletonCardProps = { - numRows: number; - - /** Draws the skeleton shapes for a single row from the geometry measured off the card */ - renderRow: (args: SkeletonRowArgs) => React.ReactNode; - - /** Whether each row gets a bottom border, matching cards whose real rows are separated */ - shouldShowSeparators?: boolean; +type HomePageSkeletonCardShellProps = { + /** What the card draws inside its container */ + children: React.ReactNode; }; -function HomePageSkeletonCard({numRows, renderRow, shouldShowSeparators = false}: HomePageSkeletonCardProps) { +function HomePageSkeletonCardShell({children}: HomePageSkeletonCardShellProps) { const styles = useThemeStyles(); const {shouldUseNarrowLayout} = useResponsiveLayout(); - const {onLayout, containerWidth} = useContainerWidth(); - const {iconTextGap, rowHeight, horizontalPadding} = useWidgetSkeletonRowGeometry(); - const textLineGap = styles.gap1.gap; return ( @@ -135,48 +122,51 @@ function HomePageSkeletonCard({numRows, renderRow, shouldShowSeparators = false} } containerStyles={styles.getWidgetContainerBottomPaddingStyle(shouldUseNarrowLayout)} > - renderRow({itemIndex, width: containerWidth, horizontalPadding, rowHeight, iconTextGap, textLineGap})} - onLayout={onLayout} - /> + {children} ); } -// The card this stands in for has no bar-representable rows, so a shimmer stand-in would invent a row -// structure the real card does not have. -function HomePageSkeletonSpinnerCard() { +type HomePageSkeletonCardProps = { + numRows: number; + + /** Draws the skeleton shapes for a single row from the geometry measured off the card */ + renderRow: (args: SkeletonRowArgs) => React.ReactNode; + + /** Whether each row gets a bottom border, matching cards whose real rows are separated */ + shouldShowSeparators?: boolean; +}; + +function HomePageSkeletonCard({numRows, renderRow, shouldShowSeparators = false}: HomePageSkeletonCardProps) { const styles = useThemeStyles(); - const StyleUtils = useStyleUtils(); - const {shouldUseNarrowLayout} = useResponsiveLayout(); + const {onLayout, containerWidth} = useContainerWidth(); + const {iconTextGap, rowHeight, horizontalPadding} = useWidgetSkeletonRowGeometry(); + const textLineGap = styles.gap1.gap; return ( - - - } - containerStyles={styles.getWidgetContainerBottomPaddingStyle(shouldUseNarrowLayout)} - > - - - - - + + renderRow({itemIndex, width: containerWidth, horizontalPadding, rowHeight, iconTextGap, textLineGap})} + onLayout={onLayout} + /> + + ); +} + +// The line shape is drawn because the default Home insight is a line chart. +function HomePageSkeletonChartCard() { + return ( + + + ); } @@ -196,4 +186,4 @@ function HomePageSkeletonRowCards() { ); } -export {HomePageSkeletonSpinnerCard, HomePageSkeletonRowCards, CARD_TEST_ID, SPINNER_TEST_ID}; +export {HomePageSkeletonChartCard, HomePageSkeletonRowCards, CARD_TEST_ID}; diff --git a/tests/unit/components/Charts/ChartSkeleton.test.tsx b/tests/unit/components/Charts/ChartSkeleton.test.tsx index 867f8b01c002..722205a1fcbb 100644 --- a/tests/unit/components/Charts/ChartSkeleton.test.tsx +++ b/tests/unit/components/Charts/ChartSkeleton.test.tsx @@ -10,8 +10,7 @@ import React from 'react'; const CONTAINER_WIDTH = 320; const NARROW_CONTAINER_WIDTH = 60; -// The shapes size off the measured container, which never lays out under the test renderer, so without this -// every shape would draw at zero width and the assertions would pass on an empty box. +// The shapes size off the measured container, which never lays out under the test renderer, so without this every shape would draw at zero width. function renderAtContainerWidth(view: ChartView, width = CONTAINER_WIDTH) { render(); fireEvent(screen.getByTestId(CHART_SKELETON_TEST_ID), 'layout', {nativeEvent: {layout: {width, height: 0}}}); diff --git a/tests/unit/pages/HomePage.test.tsx b/tests/unit/pages/HomePage.test.tsx index f9712dc9a094..8339eeb64043 100644 --- a/tests/unit/pages/HomePage.test.tsx +++ b/tests/unit/pages/HomePage.test.tsx @@ -92,19 +92,19 @@ jest.mock('@libs/NetworkState', () => ({ // Deliberately not `mockSection(...)`: that helper emits a `section-` testID, which `renderedSectionOrder` // matches, so a skeleton group would count as a rendered section. -const mockSpinnerCardTestID = 'homePageSkeletonSpinnerCard'; +const mockChartCardTestID = 'homePageSkeletonChartCard'; const mockRowCardsTestID = 'homePageSkeletonRowCards'; jest.mock('@pages/home/HomePageSkeleton', () => { const ReactModule = require('react'); const {View: RNView} = require('react-native'); - function MockHomePageSkeletonSpinnerCard() { - return ReactModule.createElement(RNView, {testID: mockSpinnerCardTestID}); + function MockHomePageSkeletonChartCard() { + return ReactModule.createElement(RNView, {testID: mockChartCardTestID}); } function MockHomePageSkeletonRowCards() { return ReactModule.createElement(RNView, {testID: mockRowCardsTestID}); } - return {HomePageSkeletonSpinnerCard: MockHomePageSkeletonSpinnerCard, HomePageSkeletonRowCards: MockHomePageSkeletonRowCards}; + return {HomePageSkeletonChartCard: MockHomePageSkeletonChartCard, HomePageSkeletonRowCards: MockHomePageSkeletonRowCards}; }); // Each section is mocked to render a stable `section-` testID so we can assert ordering and column placement. @@ -547,7 +547,7 @@ describe('HomePage', () => { const leftOrder = within(leftColumn) .getAllByTestId(/^(section-|homePageSkeleton)/) .map((el) => String(el.props.testID)); - expect(leftOrder).toEqual(['section-ForYouSection', mockSpinnerCardTestID]); + expect(leftOrder).toEqual(['section-ForYouSection', mockChartCardTestID]); expect(within(rightColumn).getByTestId(mockRowCardsTestID)).toBeOnTheScreen(); expect(renderedSectionOrder()).toEqual(['section-ForYouSection']); diff --git a/tests/unit/pages/home/HomePageSkeletonTest.tsx b/tests/unit/pages/home/HomePageSkeletonTest.tsx index 0df3d5cf0b3e..3f0bfa7dcb0d 100644 --- a/tests/unit/pages/home/HomePageSkeletonTest.tsx +++ b/tests/unit/pages/home/HomePageSkeletonTest.tsx @@ -1,24 +1,25 @@ import {render, screen} from '@testing-library/react-native'; -import {CARD_TEST_ID, HomePageSkeletonRowCards, HomePageSkeletonSpinnerCard, SPINNER_TEST_ID} from '@pages/home/HomePageSkeleton'; +import {CHART_SKELETON_TEST_ID} from '@components/Charts/ChartSkeleton'; + +import {CARD_TEST_ID, HomePageSkeletonChartCard, HomePageSkeletonRowCards} from '@pages/home/HomePageSkeleton'; import React from 'react'; -// The cards read `shouldUseNarrowLayout` for horizontal padding only, never for card count or shape. +// The cards read `shouldUseNarrowLayout` for padding only, never for card count or shape. jest.mock('@hooks/useResponsiveLayout', () => jest.fn(() => ({shouldUseNarrowLayout: false}))); -describe('HomePageSkeleton card groups', () => { - it('contributes two cards from the row group', () => { +describe('HomePageSkeleton cards', () => { + it('renders two row cards', () => { render(); expect(screen.getAllByTestId(CARD_TEST_ID)).toHaveLength(2); - expect(screen.queryByTestId(SPINNER_TEST_ID)).not.toBeOnTheScreen(); }); - it('contributes one spinner-bearing card from the spinner group', () => { - render(); + it('renders one card holding a chart placeholder', () => { + render(); expect(screen.getAllByTestId(CARD_TEST_ID)).toHaveLength(1); - expect(screen.getByTestId(SPINNER_TEST_ID)).toBeOnTheScreen(); + expect(screen.getByTestId(CHART_SKELETON_TEST_ID)).toBeOnTheScreen(); }); }); From a6d2ef7e818ad05ed244a8c78af75c77159ca495 Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Wed, 9 Sep 2026 15:12:21 +0200 Subject: [PATCH 04/15] fix: reserve the x-axis label strip in the chart placeholder --- .../Charts/BarChart/BarChartContent.tsx | 7 ++- src/components/Charts/ChartSkeleton.tsx | 18 ++++--- .../Charts/LineChart/LineChartContent.tsx | 10 ++-- src/components/Charts/VictoryTheme.ts | 51 +++++++++---------- .../components/Charts/ChartSkeleton.test.tsx | 19 +++++++ .../Charts/getCartesianChartHeight.test.ts | 13 +++++ 6 files changed, 72 insertions(+), 46 deletions(-) create mode 100644 tests/unit/components/Charts/getCartesianChartHeight.test.ts diff --git a/src/components/Charts/BarChart/BarChartContent.tsx b/src/components/Charts/BarChart/BarChartContent.tsx index 84e922e07647..689d12060cfb 100644 --- a/src/components/Charts/BarChart/BarChartContent.tsx +++ b/src/components/Charts/BarChart/BarChartContent.tsx @@ -15,7 +15,7 @@ import { useLabelHitTesting, } from '@components/Charts/hooks'; import {calculateMinDomainPadding, getXAxisLabel, getYAxisLabelWidth} from '@components/Charts/utils'; -import VictoryTheme, {CHART_CONTENT_MIN_HEIGHT, GLYPH_PADDING} from '@components/Charts/VictoryTheme'; +import VictoryTheme, {getCartesianChartHeight, getXAxisLabelSpace, GLYPH_PADDING} from '@components/Charts/VictoryTheme'; import useTheme from '@hooks/useTheme'; import useThemeStyles from '@hooks/useThemeStyles'; @@ -40,7 +40,6 @@ import type {CartesianChartProps, ChartDataPoint} from '..'; const BASE_DOMAIN_PADDING = {top: 32, bottom: 1, left: 0, right: 0}; type BarChartProps = CartesianChartProps & { - /** Callback when a bar is pressed */ onBarPress?: (dataPoint: ChartDataPoint, index: number) => void; /** When true, all bars use the same color. When false (default), each bar uses a different color from the palette. */ @@ -229,8 +228,8 @@ function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'l ); }; - const labelSpace = VictoryTheme.axis.labelGap + (xAxisLabelHeight ?? 0); - const dynamicChartStyle = {height: CHART_CONTENT_MIN_HEIGHT + labelSpace}; + const labelSpace = getXAxisLabelSpace(xAxisLabelHeight); + const dynamicChartStyle = {height: getCartesianChartHeight(xAxisLabelHeight)}; const yAxisLabelWidth = getYAxisLabelWidth(data, formatValue, fontManager, variables.iconSizeExtraSmall, BASE_DOMAIN_PADDING); const chartPadding = {...VictoryTheme.axis.padding, bottom: labelSpace + VictoryTheme.axis.padding.bottom, left: yAxisLabelWidth + GLYPH_PADDING}; diff --git a/src/components/Charts/ChartSkeleton.tsx b/src/components/Charts/ChartSkeleton.tsx index 680ffd96347e..48bb85f750c7 100644 --- a/src/components/Charts/ChartSkeleton.tsx +++ b/src/components/Charts/ChartSkeleton.tsx @@ -13,7 +13,7 @@ import React from 'react'; import {View} from 'react-native'; import {Circle, Path} from 'react-native-svg'; -import {CHART_CONTENT_MIN_HEIGHT} from './VictoryTheme'; +import {CHART_CONTENT_MIN_HEIGHT, getCartesianChartHeight} from './VictoryTheme'; const CHART_SKELETON_TEST_ID = 'chartSkeleton'; const BAR_TEST_ID = 'chartSkeletonBar'; @@ -79,20 +79,22 @@ function renderPieShape(width: number) { ); } -const SHAPE_BY_VIEW: Record ReactNode> = { - [CONST.SEARCH.VIEW.BAR]: renderBarShape, - [CONST.SEARCH.VIEW.LINE]: renderLineShape, - [CONST.SEARCH.VIEW.PIE]: renderPieShape, +// Each height is the box its chart draws into, so the box holds its size when the chart replaces the placeholder. The +// pie chart's legend sits below that box and is not stood in for, so the pie card still grows by the legend. +const PLACEHOLDER_BY_VIEW: Record ReactNode; height: number}> = { + [CONST.SEARCH.VIEW.BAR]: {render: renderBarShape, height: getCartesianChartHeight()}, + [CONST.SEARCH.VIEW.LINE]: {render: renderLineShape, height: getCartesianChartHeight()}, + [CONST.SEARCH.VIEW.PIE]: {render: renderPieShape, height: CHART_CONTENT_MIN_HEIGHT}, }; type ChartSkeletonProps = { - /** Chart view type whose placeholder shape to draw */ view: ChartView; }; function ChartSkeleton({view}: ChartSkeletonProps) { const theme = useTheme(); const {onLayout, containerWidth} = useContainerWidth(); + const {render, height} = PLACEHOLDER_BY_VIEW[view]; return ( - {SHAPE_BY_VIEW[view](containerWidth)} + {render(containerWidth)} ); diff --git a/src/components/Charts/LineChart/LineChartContent.tsx b/src/components/Charts/LineChart/LineChartContent.tsx index db537a3ca1c1..4610dc3454b1 100644 --- a/src/components/Charts/LineChart/LineChartContent.tsx +++ b/src/components/Charts/LineChart/LineChartContent.tsx @@ -17,7 +17,7 @@ import { useLabelHitTesting, } from '@components/Charts/hooks'; import {getXAxisLabel, getYAxisLabelWidth, labelOverhang} from '@components/Charts/utils'; -import VictoryTheme, {CHART_CONTENT_MIN_HEIGHT, GLYPH_PADDING, LABEL_PADDING, LABEL_ROTATIONS, SIN_45} from '@components/Charts/VictoryTheme'; +import VictoryTheme, {getCartesianChartHeight, getXAxisLabelSpace, GLYPH_PADDING, LABEL_PADDING, LABEL_ROTATIONS, SIN_45} from '@components/Charts/VictoryTheme'; import useTheme from '@hooks/useTheme'; import useThemeStyles from '@hooks/useThemeStyles'; @@ -36,17 +36,13 @@ import {CartesianChart, Line} from 'victory-native'; import type {CartesianChartProps, ChartDataPoint} from '..'; -/** Inner dot radius for line chart data points */ const DOT_RADIUS = 4; -/** Extra hover area beyond the dot radius for easier touch targeting */ const DOT_HOVER_EXTRA_RADIUS = 2; -/** Base domain padding applied to all sides */ const BASE_DOMAIN_PADDING = {top: 16, bottom: 16, left: 0, right: 0}; type LineChartProps = CartesianChartProps & { - /** Callback when a data point is pressed */ onPointPress?: (dataPoint: ChartDataPoint, index: number) => void; }; @@ -224,8 +220,8 @@ function LineChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = ' ); }; - const labelSpace = VictoryTheme.axis.labelGap + (xAxisLabelHeight ?? 0); - const dynamicChartStyle = {height: CHART_CONTENT_MIN_HEIGHT + labelSpace}; + const labelSpace = getXAxisLabelSpace(xAxisLabelHeight); + const dynamicChartStyle = {height: getCartesianChartHeight(xAxisLabelHeight)}; const chartPadding = { ...VictoryTheme.axis.padding, bottom: labelSpace + VictoryTheme.axis.padding.bottom, diff --git a/src/components/Charts/VictoryTheme.ts b/src/components/Charts/VictoryTheme.ts index b11720661f13..7c397e171dd8 100644 --- a/src/components/Charts/VictoryTheme.ts +++ b/src/components/Charts/VictoryTheme.ts @@ -1,28 +1,16 @@ -/** - * Centralized styles and layout constants for the chart components. - */ import colors from '@styles/theme/colors'; +import variables from '@styles/variables'; import {CHART_FONT_FAMILY_NAMES} from './utils/chartFontConstants'; -/** Shade groups in the palette */ +/** The shade order maximizes contrast between adjacent palette entries, keeping adjacent chart segments easy to distinguish. */ const CHART_PALETTE_SHADES = [400, 600, 300, 500, 700] as const; -/** Hues cycling within each shade group */ const CHART_PALETTE_HUES = ['yellow', 'tangerine', 'pink', 'green', 'ice', 'blue'] as const; -/** - * Expensify Chart Color Palette. - * - * Shades are ordered (400, 600, 300, 500, 700) so that sequential colors have - * maximum contrast, making adjacent chart segments easy to distinguish. - * - * Within each shade, hues cycle: Yellow, Tangerine, Pink, Green, Ice, Blue. - */ const CHART_PALETTE: string[] = (() => { const palette: string[] = []; - // Generate the 30 unique combinations (5 shades × 6 hues) for (const shade of CHART_PALETTE_SHADES) { for (const hue of CHART_PALETTE_HUES) { const colorKey = `${hue}${shade}`; @@ -35,13 +23,9 @@ const CHART_PALETTE: string[] = (() => { return palette; })(); -/** - * Gets a color from the chart palette based on index. - * Automatically loops back to the start if the index exceeds 29. - */ function getChartColor(index: number): string { if (CHART_PALETTE.length === 0) { - return colors.black; // Fallback + return colors.black; } return CHART_PALETTE.at(index % CHART_PALETTE.length) ?? colors.black; } @@ -55,9 +39,7 @@ const DEFAULT_CHART_DOT_COLOR_INDEX = DEFAULT_CHART_COLOR_INDEX + CHART_PALETTE_ const VictoryTheme = { colors: { palette: CHART_PALETTE, - /** Default color used for single-color charts (e.g., line chart, single-color bar chart) */ default: getChartColor(DEFAULT_CHART_COLOR_INDEX), - /** Default dot color for line chart data points, one shade darker than the line */ defaultDot: getChartColor(DEFAULT_CHART_DOT_COLOR_INDEX), getColor: getChartColor, }, @@ -65,19 +47,15 @@ const VictoryTheme = { axis: { /** Number of Y-axis ticks (including zero) */ tickCount: 5, - /** Line width for X-axis (hidden) */ xLineWidth: 0, /** Line width for Y-axis grid lines */ yLineWidth: 1, - /** Desired visual gap (px) between axis labels and the chart edge, used for both axes */ + /** Visual gap (px) between axis labels and the chart edge */ labelGap: 12, - /** Base chart padding applied to all sides */ padding: {top: 5, left: 5, right: 5, bottom: 5}, }, tooltip: { - /** The height of the chart tooltip pointer */ pointerHeight: 4, - /** The width of the chart tooltip pointer */ pointerWidth: 12, }, pie: { @@ -93,7 +71,24 @@ const VictoryTheme = { /** Minimum height for the chart content area (bars, Y-axis, grid lines) */ const CHART_CONTENT_MIN_HEIGHT = 250; -/** Pixel height of the y-scale output range inside CartesianChart (content height minus base axis padding). */ +/** ExpensifyNeue's ascent plus descent over its em, 950 + 230 of 1000 units, which is the line height Skia reports for label text. */ +const LABEL_LINE_HEIGHT_RATIO = 1.18; + +/** + * One horizontal line of label text. CartesianChart reports the plot bounds a measured strip needs only after a + * render, and the placeholder has no loaded font to measure, so both fall back to this floor. + */ +const X_AXIS_LABEL_MIN_HEIGHT = Math.ceil(variables.iconSizeExtraSmall * LABEL_LINE_HEIGHT_RATIO); + +function getXAxisLabelSpace(xAxisLabelHeight = 0): number { + return VictoryTheme.axis.labelGap + Math.max(xAxisLabelHeight, X_AXIS_LABEL_MIN_HEIGHT); +} + +function getCartesianChartHeight(xAxisLabelHeight = 0): number { + return CHART_CONTENT_MIN_HEIGHT + getXAxisLabelSpace(xAxisLabelHeight); +} + +/** Pixel height of the y-scale output range inside CartesianChart. */ const CHART_Y_SCALE_HEIGHT = CHART_CONTENT_MIN_HEIGHT - VictoryTheme.axis.padding.top - VictoryTheme.axis.padding.bottom; /** Supported label rotation angles in degrees */ @@ -129,6 +124,8 @@ const GLYPH_PADDING = 4; export { CHART_CONTENT_MIN_HEIGHT, CHART_Y_SCALE_HEIGHT, + getCartesianChartHeight, + getXAxisLabelSpace, LABEL_ROTATIONS, SIN_45, LABEL_PADDING, diff --git a/tests/unit/components/Charts/ChartSkeleton.test.tsx b/tests/unit/components/Charts/ChartSkeleton.test.tsx index 722205a1fcbb..e69c18597f58 100644 --- a/tests/unit/components/Charts/ChartSkeleton.test.tsx +++ b/tests/unit/components/Charts/ChartSkeleton.test.tsx @@ -1,7 +1,9 @@ import {fireEvent, render, screen} from '@testing-library/react-native'; import ChartSkeleton, {BAR_TEST_ID, CHART_SKELETON_TEST_ID, LINE_TEST_ID, PIE_TEST_ID} from '@components/Charts/ChartSkeleton'; +import {CHART_CONTENT_MIN_HEIGHT, getCartesianChartHeight} from '@components/Charts/VictoryTheme'; import type {ChartView} from '@components/Search/types'; +import SkeletonViewContentLoader from '@components/SkeletonViewContentLoader'; import CONST from '@src/CONST'; @@ -16,6 +18,10 @@ function renderAtContainerWidth(view: ChartView, width = CONTAINER_WIDTH) { fireEvent(screen.getByTestId(CHART_SKELETON_TEST_ID), 'layout', {nativeEvent: {layout: {width, height: 0}}}); } +function getSkeletonHeight() { + return Number(screen.UNSAFE_getByType(SkeletonViewContentLoader).props.height); +} + describe('ChartSkeleton', () => { it('should draw a series of bars for the bar view', () => { renderAtContainerWidth(CONST.SEARCH.VIEW.BAR); @@ -49,4 +55,17 @@ describe('ChartSkeleton', () => { expect(screen.queryByTestId(BAR_TEST_ID)).toBeNull(); expect(screen.queryByTestId(LINE_TEST_ID)).toBeNull(); }); + + it('should reserve the x-axis label strip for the cartesian views and not for the pie view', () => { + renderAtContainerWidth(CONST.SEARCH.VIEW.LINE); + expect(getSkeletonHeight()).toBe(getCartesianChartHeight()); + + screen.unmount(); + renderAtContainerWidth(CONST.SEARCH.VIEW.BAR); + expect(getSkeletonHeight()).toBe(getCartesianChartHeight()); + + screen.unmount(); + renderAtContainerWidth(CONST.SEARCH.VIEW.PIE); + expect(getSkeletonHeight()).toBe(CHART_CONTENT_MIN_HEIGHT); + }); }); diff --git a/tests/unit/components/Charts/getCartesianChartHeight.test.ts b/tests/unit/components/Charts/getCartesianChartHeight.test.ts new file mode 100644 index 000000000000..05913c6e742e --- /dev/null +++ b/tests/unit/components/Charts/getCartesianChartHeight.test.ts @@ -0,0 +1,13 @@ +import VictoryTheme, {CHART_CONTENT_MIN_HEIGHT, getCartesianChartHeight} from '@components/Charts/VictoryTheme'; + +const TALL_LABEL_HEIGHT = 120; + +describe('getCartesianChartHeight', () => { + it('should hold one height until a measured strip exceeds the floor', () => { + const floored = getCartesianChartHeight(); + + expect(getCartesianChartHeight(0)).toBe(floored); + expect(getCartesianChartHeight(1)).toBe(floored); + expect(getCartesianChartHeight(TALL_LABEL_HEIGHT)).toBe(CHART_CONTENT_MIN_HEIGHT + VictoryTheme.axis.labelGap + TALL_LABEL_HEIGHT); + }); +}); From 590bba43b3234b7c4d05536261aa6023d35fff86 Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Thu, 10 Sep 2026 17:20:25 +0200 Subject: [PATCH 05/15] feat: hand measured chart size from placeholder to chart --- jest/setup.ts | 3 + .../Charts/BarChart/BarChartContent.tsx | 29 +++--- .../Charts/LineChart/LineChartContent.tsx | 29 +++--- .../Charts/PieChart/PieChartContent.tsx | 24 ++--- src/components/Charts/hooks/index.ts | 1 + .../Charts/hooks/useMeasuredChartSize.ts | 23 +++++ .../Charts/chartPlaceholderHandover.test.tsx | 88 +++++++++++++++++++ 7 files changed, 161 insertions(+), 36 deletions(-) create mode 100644 src/components/Charts/hooks/useMeasuredChartSize.ts create mode 100644 tests/unit/components/Charts/chartPlaceholderHandover.test.tsx diff --git a/jest/setup.ts b/jest/setup.ts index 3a6464839967..86dded7b46e4 100644 --- a/jest/setup.ts +++ b/jest/setup.ts @@ -347,6 +347,9 @@ jest.mock('victory-native', () => ({ CartesianChart: jest.fn( ({children}: {children?: (args: Record) => ReactNode}) => children?.({points: {y: []}, chartBounds: {left: 0, right: 0, top: 0, bottom: 0}}) ?? null, ), + Line: jest.fn(() => null), + Pie: {Chart: jest.fn(() => null)}, + PolarChart: jest.fn(() => null), })); // Provide a default global fetch mock for tests that do not explicitly set it up diff --git a/src/components/Charts/BarChart/BarChartContent.tsx b/src/components/Charts/BarChart/BarChartContent.tsx index 689d12060cfb..2b319a0d3c5c 100644 --- a/src/components/Charts/BarChart/BarChartContent.tsx +++ b/src/components/Charts/BarChart/BarChartContent.tsx @@ -13,6 +13,7 @@ import { useChartLabelMeasurements, useDynamicYDomain, useLabelHitTesting, + useMeasuredChartSize, } from '@components/Charts/hooks'; import {calculateMinDomainPadding, getXAxisLabel, getYAxisLabelWidth} from '@components/Charts/utils'; import VictoryTheme, {getCartesianChartHeight, getXAxisLabelSpace, GLYPH_PADDING} from '@components/Charts/VictoryTheme'; @@ -24,10 +25,10 @@ import variables from '@styles/variables'; import CONST from '@src/CONST'; -import type {LayoutChangeEvent} from 'react-native'; import type {CartesianChartRenderArg, ChartBounds, PointsArray, Scale} from 'victory-native'; import React, {useState} from 'react'; +import {View} from 'react-native'; import {GestureDetector} from 'react-native-gesture-handler'; import Animated, {useAnimatedStyle, useSharedValue} from 'react-native-reanimated'; import {Bar, CartesianChart} from 'victory-native'; @@ -50,7 +51,7 @@ function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'l const theme = useTheme(); const styles = useThemeStyles(); const fontManager = useChartFontManager(); - const [chartWidth, setChartWidth] = useState(0); + const {onLayout, width: chartWidth, measuredSize} = useMeasuredChartSize(); const [barAreaWidth, setBarAreaWidth] = useState(0); const [boundsLeft, setBoundsLeft] = useState(0); const [boundsRight, setBoundsRight] = useState(0); @@ -73,10 +74,6 @@ function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'l } }; - const handleLayout = (event: LayoutChangeEvent) => { - setChartWidth(event.nativeEvent.layout.width); - }; - const domainPadding = (() => { if (chartWidth === 0) { return BASE_DOMAIN_PADDING; @@ -229,12 +226,19 @@ function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'l }; const labelSpace = getXAxisLabelSpace(xAxisLabelHeight); - const dynamicChartStyle = {height: getCartesianChartHeight(xAxisLabelHeight)}; + const chartBoxStyle = [styles.chartContent, {height: getCartesianChartHeight(xAxisLabelHeight)}]; const yAxisLabelWidth = getYAxisLabelWidth(data, formatValue, fontManager, variables.iconSizeExtraSmall, BASE_DOMAIN_PADDING); const chartPadding = {...VictoryTheme.axis.padding, bottom: labelSpace + VictoryTheme.axis.padding.bottom, left: yAxisLabelWidth + GLYPH_PADDING}; if (isLoading || !fontManager) { - return ; + return ( + + + + ); } if (data.length === 0) { @@ -244,11 +248,12 @@ function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'l return ( - {chartWidth > 0 && ( + {measuredSize ? ( { - setChartWidth(event.nativeEvent.layout.width); - }; - const chartBottom = useSharedValue(0); const measurements = useChartLabelMeasurements(data, fontManager, variables.iconSizeExtraSmall); @@ -221,7 +218,7 @@ function LineChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = ' }; const labelSpace = getXAxisLabelSpace(xAxisLabelHeight); - const dynamicChartStyle = {height: getCartesianChartHeight(xAxisLabelHeight)}; + const chartBoxStyle = [styles.chartContent, {height: getCartesianChartHeight(xAxisLabelHeight)}]; const chartPadding = { ...VictoryTheme.axis.padding, bottom: labelSpace + VictoryTheme.axis.padding.bottom, @@ -229,7 +226,14 @@ function LineChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = ' }; if (isLoading || !fontManager) { - return ; + return ( + + + + ); } if (data.length === 0) { @@ -239,11 +243,12 @@ function LineChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = ' return ( - {chartWidth > 0 && ( + {measuredSize ? ( { - setCanvasWidth(event.nativeEvent.layout.width); - setCanvasHeight(event.nativeEvent.layout.height); - }; - // Calculate pie geometry const radius = Math.min(canvasWidth, canvasHeight) / 2; const innerRadius = radius * VictoryTheme.pie.innerRadiusRatio; @@ -153,7 +145,14 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice }; if (isLoading) { - return ; + return ( + + + + ); } if (data.length === 0) { @@ -165,10 +164,11 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice {processedSlices.length > 0 && ( { + setSize({width: event.nativeEvent.layout.width, height: event.nativeEvent.layout.height}); + }; + + const measuredSize: ChartExplicitSize | undefined = size.width > 0 && size.height > 0 ? size : undefined; + + return {onLayout, width: size.width, height: size.height, measuredSize}; +} + +export default useMeasuredChartSize; diff --git a/tests/unit/components/Charts/chartPlaceholderHandover.test.tsx b/tests/unit/components/Charts/chartPlaceholderHandover.test.tsx new file mode 100644 index 000000000000..b7818422a47a --- /dev/null +++ b/tests/unit/components/Charts/chartPlaceholderHandover.test.tsx @@ -0,0 +1,88 @@ +import {fireEvent, render, screen} from '@testing-library/react-native'; + +import BarChartContent from '@components/Charts/BarChart/BarChartContent'; +import {CHART_SKELETON_TEST_ID} from '@components/Charts/ChartSkeleton'; +import LineChartContent from '@components/Charts/LineChart/LineChartContent'; +import PieChartContent from '@components/Charts/PieChart/PieChartContent'; +import type {ChartDataPoint} from '@components/Charts/types'; + +import React from 'react'; +import {PolarChart} from 'victory-native'; + +const CONTAINER_WIDTH = 320; +const CONTAINER_HEIGHT = 240; + +const data: ChartDataPoint[] = [ + {label: 'Amazon', total: 120}, + {label: 'Uber', total: 80}, +]; + +/** The placeholder measures its own container as well, so the box under test is the outermost ancestor handling layout. */ +function getMeasuredBox() { + let node = screen.getByTestId(CHART_SKELETON_TEST_ID).parent; + let measuredBox = null as typeof node; + + while (node) { + if (typeof node.props.onLayout === 'function') { + measuredBox = node; + } + node = node.parent; + } + + if (!measuredBox) { + throw new Error('the placeholder has no ancestor reporting its layout'); + } + + return measuredBox; +} + +describe('chart placeholder handover', () => { + afterEach(() => { + jest.clearAllMocks(); + }); + + it.each([ + ['bar', BarChartContent], + ['line', LineChartContent], + ['pie', PieChartContent], + ])('should measure the %s chart box around its placeholder', (_name, ChartContent) => { + render( + , + ); + + expect(getMeasuredBox()).toBeTruthy(); + }); + + it('should hand the pie chart the size measured around its placeholder', () => { + const {rerender} = render( + , + ); + + fireEvent(getMeasuredBox(), 'layout', {nativeEvent: {layout: {width: CONTAINER_WIDTH, height: CONTAINER_HEIGHT}}}); + rerender( + , + ); + + expect(PolarChart).toHaveBeenCalledWith(expect.objectContaining({explicitSize: {width: CONTAINER_WIDTH, height: CONTAINER_HEIGHT}}), undefined); + }); + + it('should hand the pie chart no size before one has been measured', () => { + render( + , + ); + + expect(PolarChart).toHaveBeenCalledWith(expect.objectContaining({explicitSize: undefined}), undefined); + }); +}); From b3c60cc40729f21c4e6b76bd17a111356c198156 Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Thu, 10 Sep 2026 19:02:57 +0200 Subject: [PATCH 06/15] feat: measure chart width above the lazy chart boundary --- .../Charts/BarChart/BarChartContent.tsx | 31 ++++++------- .../Charts/BarChart/index.native.tsx | 13 +++++- src/components/Charts/BarChart/index.tsx | 15 ++++-- src/components/Charts/ChartWidthBox.tsx | 28 +++++++++++ .../Charts/LineChart/LineChartContent.tsx | 31 ++++++------- .../Charts/LineChart/index.native.tsx | 13 +++++- src/components/Charts/LineChart/index.tsx | 15 ++++-- .../Charts/PieChart/PieChartContent.tsx | 46 ++++++++----------- .../Charts/PieChart/index.native.tsx | 13 +++++- src/components/Charts/PieChart/index.tsx | 15 ++++-- src/components/Charts/hooks/index.ts | 1 - .../Charts/hooks/useMeasuredChartSize.ts | 23 ---------- .../Charts/chartPlaceholderHandover.test.tsx | 32 ++++++------- 13 files changed, 159 insertions(+), 117 deletions(-) create mode 100644 src/components/Charts/ChartWidthBox.tsx delete mode 100644 src/components/Charts/hooks/useMeasuredChartSize.ts diff --git a/src/components/Charts/BarChart/BarChartContent.tsx b/src/components/Charts/BarChart/BarChartContent.tsx index 2b319a0d3c5c..b61cef206cf6 100644 --- a/src/components/Charts/BarChart/BarChartContent.tsx +++ b/src/components/Charts/BarChart/BarChartContent.tsx @@ -13,7 +13,6 @@ import { useChartLabelMeasurements, useDynamicYDomain, useLabelHitTesting, - useMeasuredChartSize, } from '@components/Charts/hooks'; import {calculateMinDomainPadding, getXAxisLabel, getYAxisLabelWidth} from '@components/Charts/utils'; import VictoryTheme, {getCartesianChartHeight, getXAxisLabelSpace, GLYPH_PADDING} from '@components/Charts/VictoryTheme'; @@ -47,11 +46,14 @@ type BarChartProps = CartesianChartProps & { useSingleColor?: boolean; }; -function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'left', useSingleColor = false, onBarPress}: BarChartProps) { +type BarChartContentProps = BarChartProps & { + chartWidth: number; +}; + +function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'left', useSingleColor = false, onBarPress, chartWidth}: BarChartContentProps) { const theme = useTheme(); const styles = useThemeStyles(); const fontManager = useChartFontManager(); - const {onLayout, width: chartWidth, measuredSize} = useMeasuredChartSize(); const [barAreaWidth, setBarAreaWidth] = useState(0); const [boundsLeft, setBoundsLeft] = useState(0); const [boundsRight, setBoundsRight] = useState(0); @@ -226,16 +228,16 @@ function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'l }; const labelSpace = getXAxisLabelSpace(xAxisLabelHeight); - const chartBoxStyle = [styles.chartContent, {height: getCartesianChartHeight(xAxisLabelHeight)}]; + const chartHeight = getCartesianChartHeight(xAxisLabelHeight); + const chartBoxStyle = [styles.chartContent, {height: chartHeight}]; + + const chartSize = chartWidth > 0 ? {width: chartWidth, height: chartHeight} : undefined; const yAxisLabelWidth = getYAxisLabelWidth(data, formatValue, fontManager, variables.iconSizeExtraSmall, BASE_DOMAIN_PADDING); const chartPadding = {...VictoryTheme.axis.padding, bottom: labelSpace + VictoryTheme.axis.padding.bottom, left: yAxisLabelWidth + GLYPH_PADDING}; if (isLoading || !fontManager) { return ( - + ); @@ -247,13 +249,10 @@ function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'l return ( - - {measuredSize ? ( + + {chartSize ? ( ; + return ( + + {(chartWidth) => ( + + )} + + ); } BarChart.displayName = 'BarChart'; diff --git a/src/components/Charts/BarChart/index.tsx b/src/components/Charts/BarChart/index.tsx index bfbf47f9b4f8..0fca7554de14 100644 --- a/src/components/Charts/BarChart/index.tsx +++ b/src/components/Charts/BarChart/index.tsx @@ -1,4 +1,5 @@ import ChartSkeleton from '@components/Charts/ChartSkeleton'; +import ChartWidthBox from '@components/Charts/ChartWidthBox'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; import CONST from '@src/CONST'; @@ -10,11 +11,15 @@ import type {BarChartProps} from './BarChartContent'; const getBarChartContent = () => import('./BarChartContent'); function BarChart(props: BarChartProps) { return ( - } - /> + + {(chartWidth) => ( + } + /> + )} + ); } diff --git a/src/components/Charts/ChartWidthBox.tsx b/src/components/Charts/ChartWidthBox.tsx new file mode 100644 index 000000000000..1c748a5c0ec2 --- /dev/null +++ b/src/components/Charts/ChartWidthBox.tsx @@ -0,0 +1,28 @@ +import useContainerWidth from '@hooks/useContainerWidth'; +import useThemeStyles from '@hooks/useThemeStyles'; + +import type {ReactNode} from 'react'; + +import React from 'react'; +import {View} from 'react-native'; + +type ChartWidthBoxProps = { + children: (chartWidth: number) => ReactNode; +}; + +/** On web the chart component mounts only once the chart engine has downloaded, so measuring here keeps the width across that mount. */ +function ChartWidthBox({children}: ChartWidthBoxProps) { + const styles = useThemeStyles(); + const {onLayout, containerWidth} = useContainerWidth(); + + return ( + + {children(containerWidth)} + + ); +} + +export default ChartWidthBox; diff --git a/src/components/Charts/LineChart/LineChartContent.tsx b/src/components/Charts/LineChart/LineChartContent.tsx index 5c6637b7bb93..1f7a028fb8a1 100644 --- a/src/components/Charts/LineChart/LineChartContent.tsx +++ b/src/components/Charts/LineChart/LineChartContent.tsx @@ -15,7 +15,6 @@ import { useChartLabelMeasurements, useDynamicYDomain, useLabelHitTesting, - useMeasuredChartSize, } from '@components/Charts/hooks'; import {getXAxisLabel, getYAxisLabelWidth, labelOverhang} from '@components/Charts/utils'; import VictoryTheme, {getCartesianChartHeight, getXAxisLabelSpace, GLYPH_PADDING, LABEL_PADDING, LABEL_ROTATIONS, SIN_45} from '@components/Charts/VictoryTheme'; @@ -47,11 +46,14 @@ type LineChartProps = CartesianChartProps & { onPointPress?: (dataPoint: ChartDataPoint, index: number) => void; }; -function LineChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'left', onPointPress}: LineChartProps) { +type LineChartContentProps = LineChartProps & { + chartWidth: number; +}; + +function LineChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'left', onPointPress, chartWidth}: LineChartContentProps) { const theme = useTheme(); const styles = useThemeStyles(); const fontManager = useChartFontManager(); - const {onLayout, width: chartWidth, measuredSize} = useMeasuredChartSize(); const [plotAreaWidth, setPlotAreaWidth] = useState(0); const [boundsLeft, setBoundsLeft] = useState(0); const [boundsRight, setBoundsRight] = useState(0); @@ -218,7 +220,10 @@ function LineChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = ' }; const labelSpace = getXAxisLabelSpace(xAxisLabelHeight); - const chartBoxStyle = [styles.chartContent, {height: getCartesianChartHeight(xAxisLabelHeight)}]; + const chartHeight = getCartesianChartHeight(xAxisLabelHeight); + const chartBoxStyle = [styles.chartContent, {height: chartHeight}]; + + const chartSize = chartWidth > 0 ? {width: chartWidth, height: chartHeight} : undefined; const chartPadding = { ...VictoryTheme.axis.padding, bottom: labelSpace + VictoryTheme.axis.padding.bottom, @@ -227,10 +232,7 @@ function LineChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = ' if (isLoading || !fontManager) { return ( - + ); @@ -242,13 +244,10 @@ function LineChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = ' return ( - - {measuredSize ? ( + + {chartSize ? ( ; + return ( + + {(chartWidth) => ( + + )} + + ); } LineChart.displayName = 'LineChart'; diff --git a/src/components/Charts/LineChart/index.tsx b/src/components/Charts/LineChart/index.tsx index f48b6912fb6e..a2422f21482a 100644 --- a/src/components/Charts/LineChart/index.tsx +++ b/src/components/Charts/LineChart/index.tsx @@ -1,4 +1,5 @@ import ChartSkeleton from '@components/Charts/ChartSkeleton'; +import ChartWidthBox from '@components/Charts/ChartWidthBox'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; import CONST from '@src/CONST'; @@ -10,11 +11,15 @@ import type {LineChartProps} from './LineChartContent'; const getLineChartContent = () => import('./LineChartContent'); function LineChart(props: LineChartProps) { return ( - } - /> + + {(chartWidth) => ( + } + /> + )} + ); } diff --git a/src/components/Charts/PieChart/PieChartContent.tsx b/src/components/Charts/PieChart/PieChartContent.tsx index 885e4f856a2b..8046569ca7b0 100644 --- a/src/components/Charts/PieChart/PieChartContent.tsx +++ b/src/components/Charts/PieChart/PieChartContent.tsx @@ -1,9 +1,9 @@ import ChartSkeleton from '@components/Charts/ChartSkeleton'; import ChartTooltip from '@components/Charts/components/ChartTooltip'; -import {TOOLTIP_BAR_GAP, useChartLabelFormats, useMeasuredChartSize, useTooltipData} from '@components/Charts/hooks'; +import {TOOLTIP_BAR_GAP, useChartLabelFormats, useTooltipData} from '@components/Charts/hooks'; import type {ChartDataPoint, ChartProps, PieSlice, UnitPosition} from '@components/Charts/types'; import {findSliceAtPosition, processDataIntoSlices} from '@components/Charts/utils'; -import VictoryTheme from '@components/Charts/VictoryTheme'; +import VictoryTheme, {CHART_CONTENT_MIN_HEIGHT} from '@components/Charts/VictoryTheme'; import Text from '@components/Text'; import useLocalize from '@hooks/useLocalize'; @@ -21,33 +21,37 @@ import {Pie, PolarChart} from 'victory-native'; import PaddedPieSlice from './PaddedPieSlice'; type PieChartProps = ChartProps & { - /** Callback when a slice is pressed */ onSlicePress?: (dataPoint: ChartDataPoint, index: number) => void; /** Symbol/unit for value labels in tooltip (e.g., '$', '€'). */ valueUnit?: string; - /** Position of the unit symbol relative to the value. Defaults to 'left'. */ + /** Defaults to 'left'. */ valueUnitPosition?: UnitPosition; }; -function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlicePress}: PieChartProps) { +type PieChartContentProps = PieChartProps & { + chartWidth: number; +}; + +function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlicePress, chartWidth}: PieChartContentProps) { const styles = useThemeStyles(); const {translate} = useLocalize(); - const {onLayout, width: canvasWidth, height: canvasHeight, measuredSize} = useMeasuredChartSize(); const [activeSliceIndex, setActiveSliceIndex] = useState(-1); const [isHoveringOverPie, setIsHoveringOverPie] = useState(false); - // Shared values for hover state const isHovering = useSharedValue(false); const cursorX = useSharedValue(0); const cursorY = useSharedValue(0); const tooltipPosition = useSharedValue({x: 0, y: 0}); - // Calculate pie geometry - const radius = Math.min(canvasWidth, canvasHeight) / 2; + const canvasHeight = CHART_CONTENT_MIN_HEIGHT; + const radius = Math.min(chartWidth, canvasHeight) / 2; const innerRadius = radius * VictoryTheme.pie.innerRadiusRatio; - const pieGeometry = {radius, innerRadius, centerX: canvasWidth / 2, centerY: canvasHeight / 2}; + const pieGeometry = {radius, innerRadius, centerX: chartWidth / 2, centerY: canvasHeight / 2}; + + // The chart draws nothing until it has a size, so handing it one lets it draw on the render it mounts in. + const chartSize = chartWidth > 0 ? {width: chartWidth, height: canvasHeight} : undefined; // Slices are sorted by absolute value (largest first) for color assignment, // so slice indices don't match the original data array. We map back via @@ -58,7 +62,6 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice const {formatValue} = useChartLabelFormats({data, unit: valueUnit, unitPosition: valueUnitPosition}); const tooltipData = useTooltipData(activeOriginalDataIndex, data, formatValue); - // Handle hover state updates const updateActiveSlice = (x: number, y: number) => { const {centerX, centerY} = pieGeometry; const sliceIndex = findSliceAtPosition(x, y, centerX, centerY, radius, innerRadius, processedSlices); @@ -66,7 +69,6 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice setIsHoveringOverPie(sliceIndex >= 0); }; - // Handle slice press callback const handleSlicePress = (sliceIndex: number) => { if (sliceIndex < 0 || sliceIndex >= processedSlices.length) { return; @@ -81,7 +83,6 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice } }; - // Hover gesture const hoverGesture = () => Gesture.Hover() .onBegin((e) => { @@ -109,7 +110,6 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice scheduleOnRN(setIsHoveringOverPie, false); }); - // Tap gesture for click/tap navigation const tapGesture = () => Gesture.Tap().onEnd((e) => { 'worklet'; @@ -122,7 +122,6 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice } }); - // Combined gestures - Race allows both hover and tap to work independently const combinedGesture = Gesture.Race(hoverGesture(), tapGesture()); const renderLegendItem = (slice: PieSlice) => { @@ -146,10 +145,7 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice if (isLoading) { return ( - + ); @@ -162,13 +158,10 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice return ( <> - + {processedSlices.length > 0 && ( )} - {/* Tooltip */} {activeSliceIndex >= 0 && !!tooltipData && ( )} @@ -222,4 +214,4 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice } export default PieChartContent; -export type {PieChartProps}; +export type {PieChartProps, PieChartContentProps}; diff --git a/src/components/Charts/PieChart/index.native.tsx b/src/components/Charts/PieChart/index.native.tsx index 961f5bdfb82c..0bc22152239a 100644 --- a/src/components/Charts/PieChart/index.native.tsx +++ b/src/components/Charts/PieChart/index.native.tsx @@ -1,3 +1,5 @@ +import ChartWidthBox from '@components/Charts/ChartWidthBox'; + import React from 'react'; import type {PieChartProps} from './PieChartContent'; @@ -5,7 +7,16 @@ import type {PieChartProps} from './PieChartContent'; import PieChartContent from './PieChartContent'; function PieChart(props: PieChartProps) { - return ; + return ( + + {(chartWidth) => ( + + )} + + ); } PieChart.displayName = 'PieChart'; diff --git a/src/components/Charts/PieChart/index.tsx b/src/components/Charts/PieChart/index.tsx index d4f8503a4243..d8864602aea2 100644 --- a/src/components/Charts/PieChart/index.tsx +++ b/src/components/Charts/PieChart/index.tsx @@ -1,4 +1,5 @@ import ChartSkeleton from '@components/Charts/ChartSkeleton'; +import ChartWidthBox from '@components/Charts/ChartWidthBox'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; import CONST from '@src/CONST'; @@ -11,11 +12,15 @@ const getPieChartContent = () => import('./PieChartContent'); function PieChart(props: PieChartProps) { return ( - } - /> + + {(chartWidth) => ( + } + /> + )} + ); } diff --git a/src/components/Charts/hooks/index.ts b/src/components/Charts/hooks/index.ts index 61f1a6142358..5fa23151604b 100644 --- a/src/components/Charts/hooks/index.ts +++ b/src/components/Charts/hooks/index.ts @@ -9,4 +9,3 @@ export {default as useChartLabelFormats} from './useChartLabelFormats'; export {default as useDynamicYDomain} from './useDynamicYDomain'; export {default as useTooltipData} from './useTooltipData'; export {default as useLabelHitTesting} from './useLabelHitTesting'; -export {default as useMeasuredChartSize} from './useMeasuredChartSize'; diff --git a/src/components/Charts/hooks/useMeasuredChartSize.ts b/src/components/Charts/hooks/useMeasuredChartSize.ts deleted file mode 100644 index 99440853bda5..000000000000 --- a/src/components/Charts/hooks/useMeasuredChartSize.ts +++ /dev/null @@ -1,23 +0,0 @@ -import type {LayoutChangeEvent} from 'react-native'; -import type {ChartExplicitSize} from 'victory-native'; - -import {useState} from 'react'; - -/** - * A victory-native chart draws nothing until it has a size, so handing it the measured one lets it draw on the render - * it mounts in instead of waiting for a layout pass of its own. Passing a size also stops the chart listening to its - * own layout events, which makes this the only place a resize can arrive from. - */ -function useMeasuredChartSize() { - const [size, setSize] = useState({width: 0, height: 0}); - - const onLayout = (event: LayoutChangeEvent) => { - setSize({width: event.nativeEvent.layout.width, height: event.nativeEvent.layout.height}); - }; - - const measuredSize: ChartExplicitSize | undefined = size.width > 0 && size.height > 0 ? size : undefined; - - return {onLayout, width: size.width, height: size.height, measuredSize}; -} - -export default useMeasuredChartSize; diff --git a/tests/unit/components/Charts/chartPlaceholderHandover.test.tsx b/tests/unit/components/Charts/chartPlaceholderHandover.test.tsx index b7818422a47a..e69e53b51172 100644 --- a/tests/unit/components/Charts/chartPlaceholderHandover.test.tsx +++ b/tests/unit/components/Charts/chartPlaceholderHandover.test.tsx @@ -1,16 +1,16 @@ import {fireEvent, render, screen} from '@testing-library/react-native'; -import BarChartContent from '@components/Charts/BarChart/BarChartContent'; +import BarChart from '@components/Charts/BarChart'; import {CHART_SKELETON_TEST_ID} from '@components/Charts/ChartSkeleton'; -import LineChartContent from '@components/Charts/LineChart/LineChartContent'; -import PieChartContent from '@components/Charts/PieChart/PieChartContent'; +import LineChart from '@components/Charts/LineChart'; +import PieChart from '@components/Charts/PieChart'; import type {ChartDataPoint} from '@components/Charts/types'; +import {CHART_CONTENT_MIN_HEIGHT} from '@components/Charts/VictoryTheme'; import React from 'react'; import {PolarChart} from 'victory-native'; const CONTAINER_WIDTH = 320; -const CONTAINER_HEIGHT = 240; const data: ChartDataPoint[] = [ {label: 'Amazon', total: 120}, @@ -42,12 +42,12 @@ describe('chart placeholder handover', () => { }); it.each([ - ['bar', BarChartContent], - ['line', LineChartContent], - ['pie', PieChartContent], - ])('should measure the %s chart box around its placeholder', (_name, ChartContent) => { + ['bar', BarChart], + ['line', LineChart], + ['pie', PieChart], + ])('should measure the %s chart box around its placeholder', (_name, Chart) => { render( - , @@ -56,28 +56,28 @@ describe('chart placeholder handover', () => { expect(getMeasuredBox()).toBeTruthy(); }); - it('should hand the pie chart the size measured around its placeholder', () => { + it('should hand the pie chart the width measured around its placeholder', () => { const {rerender} = render( - , ); - fireEvent(getMeasuredBox(), 'layout', {nativeEvent: {layout: {width: CONTAINER_WIDTH, height: CONTAINER_HEIGHT}}}); + fireEvent(getMeasuredBox(), 'layout', {nativeEvent: {layout: {width: CONTAINER_WIDTH, height: CHART_CONTENT_MIN_HEIGHT}}}); rerender( - , ); - expect(PolarChart).toHaveBeenCalledWith(expect.objectContaining({explicitSize: {width: CONTAINER_WIDTH, height: CONTAINER_HEIGHT}}), undefined); + expect(PolarChart).toHaveBeenCalledWith(expect.objectContaining({explicitSize: {width: CONTAINER_WIDTH, height: CHART_CONTENT_MIN_HEIGHT}}), undefined); }); - it('should hand the pie chart no size before one has been measured', () => { + it('should hand the pie chart no size before a width has been measured', () => { render( - , From 72b0e2fb07b0a7ffa4ec7c5688c626ca9a1927df Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Thu, 10 Sep 2026 19:31:59 +0200 Subject: [PATCH 07/15] feat: derive chart plot bounds from the measured width --- .../tests/plotBoundsDrift.test.ts | 76 +++++++++++++++++++ .../tests/probes/plotBoundsProbe.tsx | 12 +++ .../tests/probes/plotBoundsProbeMain.tsx | 68 +++++++++++++++++ .../Charts/BarChart/BarChartContent.tsx | 39 ++++------ .../Charts/LineChart/LineChartContent.tsx | 21 ++--- src/components/Charts/utils/index.ts | 15 ++++ 6 files changed, 193 insertions(+), 38 deletions(-) create mode 100644 server/victory-chart-renderer/tests/plotBoundsDrift.test.ts create mode 100644 server/victory-chart-renderer/tests/probes/plotBoundsProbe.tsx create mode 100644 server/victory-chart-renderer/tests/probes/plotBoundsProbeMain.tsx diff --git a/server/victory-chart-renderer/tests/plotBoundsDrift.test.ts b/server/victory-chart-renderer/tests/plotBoundsDrift.test.ts new file mode 100644 index 000000000000..fdade39d3482 --- /dev/null +++ b/server/victory-chart-renderer/tests/plotBoundsDrift.test.ts @@ -0,0 +1,76 @@ +import {beforeAll, describe, expect, test} from 'bun:test'; + +import createRnStubPlugin from '@server/plugins/rnStubPlugin'; +import {spawnSync} from 'node:child_process'; +import {mkdtempSync} from 'node:fs'; +import {tmpdir} from 'node:os'; +import {basename, join, resolve} from 'node:path'; + +import {packageRoot} from './testUtils'; + +type PlotBounds = {left: number; right: number}; + +type ProbeOutput = { + predicted: PlotBounds; + reported: PlotBounds; +}; + +const runDir = mkdtempSync(join(tmpdir(), 'vcr-plot-bounds-')); +const bundlePath = join(runDir, 'plotBoundsProbe.js'); + +function hasPlotBounds(value: unknown): value is PlotBounds { + return typeof value === 'object' && value !== null && 'left' in value && typeof value.left === 'number' && 'right' in value && typeof value.right === 'number'; +} + +function parseProbeOutput(raw: string): ProbeOutput { + const parsed: unknown = JSON.parse(raw); + + if (typeof parsed !== 'object' || parsed === null || !('predicted' in parsed) || !('reported' in parsed) || !hasPlotBounds(parsed.predicted) || !hasPlotBounds(parsed.reported)) { + throw new Error(`Plot bounds probe printed unexpected output:\n${raw}`); + } + + return {predicted: parsed.predicted, reported: parsed.reported}; +} + +let probeOutput: ProbeOutput; + +describe('cartesian plot bounds', () => { + beforeAll(async () => { + const buildResult = await Bun.build({ + entrypoints: [join(import.meta.dir, 'probes/plotBoundsProbe.tsx')], + target: 'bun', + packages: 'bundle', + conditions: ['react-native'], + tsconfig: join(packageRoot, 'tsconfig.json'), + plugins: [createRnStubPlugin(resolve(packageRoot, '../stubs'))], + }); + + if (!buildResult.success) { + throw new Error(`Failed to bundle the plot bounds probe:\n${buildResult.logs.map(String).join('\n')}`); + } + + if (buildResult.outputs.length === 0) { + throw new Error('Bundled plot bounds probe output is missing'); + } + + // The bundle loads CanvasKit's wasm from beside itself, so every output has to land in the run directory. + for (const output of buildResult.outputs) { + await Bun.write(output.kind === 'entry-point' ? bundlePath : join(runDir, basename(output.path)), output); + } + + const runResult = spawnSync(process.execPath, [bundlePath], { + cwd: resolve(packageRoot, '../..'), + encoding: 'utf8', + }); + + if (runResult.status !== 0) { + throw new Error(`Plot bounds probe failed:\n${runResult.stderr}\n${runResult.stdout}`); + } + + probeOutput = parseProbeOutput(runResult.stdout); + }, 120_000); + + test('getCartesianPlotBounds matches the bounds victory-native reports', () => { + expect(probeOutput.predicted).toEqual(probeOutput.reported); + }); +}); diff --git a/server/victory-chart-renderer/tests/probes/plotBoundsProbe.tsx b/server/victory-chart-renderer/tests/probes/plotBoundsProbe.tsx new file mode 100644 index 000000000000..c4c83831a9d2 --- /dev/null +++ b/server/victory-chart-renderer/tests/probes/plotBoundsProbe.tsx @@ -0,0 +1,12 @@ +/* + * Renders one CartesianChart in a headless surface and prints, as JSON, both the plot bounds victory-native reports + * and the ones getCartesianPlotBounds predicts for the same width. + * + * It runs as a bundled child process rather than inside the test process because victory-native resolves + * react-native, which only loads through the stubs rnStubPlugin substitutes at bundle time. + */ +import initSkiaForCli from '../../src/initSkiaForCli'; + +// Skia must be initialized before the chart module and its transitive imports are evaluated. +await initSkiaForCli(); +await import('./plotBoundsProbeMain'); diff --git a/server/victory-chart-renderer/tests/probes/plotBoundsProbeMain.tsx b/server/victory-chart-renderer/tests/probes/plotBoundsProbeMain.tsx new file mode 100644 index 000000000000..f14413e0e19a --- /dev/null +++ b/server/victory-chart-renderer/tests/probes/plotBoundsProbeMain.tsx @@ -0,0 +1,68 @@ +import {getCartesianPlotBounds} from '@components/Charts/utils'; +import VictoryTheme from '@components/Charts/VictoryTheme'; + +import type {ChartBounds} from 'victory-native'; + +import {drawOffscreen, makeOffscreenSurface} from '@shopify/react-native-skia/lib/module/headless'; +import React from 'react'; +import {CartesianChart} from 'victory-native'; + +const CHART_WIDTH = 400; +const CHART_HEIGHT = 250; + +/** Stands in for the space a chart leaves for its own y-axis labels. */ +const CHART_PADDING_LEFT = 37; + +const DATA = [ + {x: 0, y: 120}, + {x: 1, y: 80}, + {x: 2, y: 45}, +]; + +const captured: {bounds: ChartBounds | null} = {bounds: null}; + +const chartElement = ( + + {({chartBounds}) => { + // The bounds reach onChartBoundsChange from an effect, which the offscreen draw never flushes, so they + // are read here instead. Both come from the same value the chart computes for its own layout. + captured.bounds = chartBounds; + return null; + }} + +); + +using surface = makeOffscreenSurface(CHART_WIDTH, CHART_HEIGHT); +using image = await drawOffscreen(surface, chartElement); + +if (!image) { + throw new Error('Skia failed to draw the probe chart'); +} + +if (!captured.bounds) { + throw new Error('CartesianChart never reported its bounds'); +} + +const predicted = getCartesianPlotBounds(CHART_WIDTH, CHART_PADDING_LEFT); + +console.log(JSON.stringify({predicted, reported: {left: captured.bounds.left, right: captured.bounds.right}})); diff --git a/src/components/Charts/BarChart/BarChartContent.tsx b/src/components/Charts/BarChart/BarChartContent.tsx index b61cef206cf6..4a72cef13689 100644 --- a/src/components/Charts/BarChart/BarChartContent.tsx +++ b/src/components/Charts/BarChart/BarChartContent.tsx @@ -14,7 +14,7 @@ import { useDynamicYDomain, useLabelHitTesting, } from '@components/Charts/hooks'; -import {calculateMinDomainPadding, getXAxisLabel, getYAxisLabelWidth} from '@components/Charts/utils'; +import {calculateMinDomainPadding, getCartesianPlotBounds, getXAxisLabel, getYAxisLabelWidth} from '@components/Charts/utils'; import VictoryTheme, {getCartesianChartHeight, getXAxisLabelSpace, GLYPH_PADDING} from '@components/Charts/VictoryTheme'; import useTheme from '@hooks/useTheme'; @@ -26,7 +26,7 @@ import CONST from '@src/CONST'; import type {CartesianChartRenderArg, ChartBounds, PointsArray, Scale} from 'victory-native'; -import React, {useState} from 'react'; +import React from 'react'; import {View} from 'react-native'; import {GestureDetector} from 'react-native-gesture-handler'; import Animated, {useAnimatedStyle, useSharedValue} from 'react-native-reanimated'; @@ -54,9 +54,6 @@ function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'l const theme = useTheme(); const styles = useThemeStyles(); const fontManager = useChartFontManager(); - const [barAreaWidth, setBarAreaWidth] = useState(0); - const [boundsLeft, setBoundsLeft] = useState(0); - const [boundsRight, setBoundsRight] = useState(0); const defaultBarColor = VictoryTheme.colors.default; const chartData = data.map((point, index) => ({ @@ -76,6 +73,17 @@ function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'l } }; + const {formatValue} = useChartLabelFormats({ + data, + unit: yAxisUnit, + unitPosition: yAxisUnitPosition, + }); + + const yAxisLabelWidth = getYAxisLabelWidth(data, formatValue, fontManager, variables.iconSizeExtraSmall, BASE_DOMAIN_PADDING); + const chartPaddingLeft = yAxisLabelWidth + GLYPH_PADDING; + const {left: boundsLeft, right: boundsRight} = getCartesianPlotBounds(chartWidth, chartPaddingLeft); + const barAreaWidth = boundsRight - boundsLeft; + const domainPadding = (() => { if (chartWidth === 0) { return BASE_DOMAIN_PADDING; @@ -102,12 +110,6 @@ function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'l measurements, }); - const {formatValue} = useChartLabelFormats({ - data, - unit: yAxisUnit, - unitPosition: yAxisUnitPosition, - }); - const barWidth = useSharedValue(0); const chartBottom = useSharedValue(0); const yZero = useSharedValue(0); @@ -121,16 +123,6 @@ function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'l chartBottom, }); - const handleChartBoundsChange = (bounds: ChartBounds) => { - const domainWidth = bounds.right - bounds.left; - const calculatedBarWidth = ((1 - BAR_INNER_PADDING) * domainWidth) / data.length; - barWidth.set(calculatedBarWidth); - yZero.set(0); - setBarAreaWidth(domainWidth); - setBoundsLeft(bounds.left); - setBoundsRight(bounds.right); - }; - const checkIsOverBar = (args: HitTestArgs) => { 'worklet'; @@ -158,6 +150,7 @@ function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'l }); const handleScaleChange = (xScale: Scale, yScale: Scale) => { + barWidth.set(((1 - BAR_INNER_PADDING) * barAreaWidth) / data.length); yZero.set(yScale(0)); updateTickPositions(xScale, data.length); setPointPositions( @@ -232,8 +225,7 @@ function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'l const chartBoxStyle = [styles.chartContent, {height: chartHeight}]; const chartSize = chartWidth > 0 ? {width: chartWidth, height: chartHeight} : undefined; - const yAxisLabelWidth = getYAxisLabelWidth(data, formatValue, fontManager, variables.iconSizeExtraSmall, BASE_DOMAIN_PADDING); - const chartPadding = {...VictoryTheme.axis.padding, bottom: labelSpace + VictoryTheme.axis.padding.bottom, left: yAxisLabelWidth + GLYPH_PADDING}; + const chartPadding = {...VictoryTheme.axis.padding, bottom: labelSpace + VictoryTheme.axis.padding.bottom, left: chartPaddingLeft}; if (isLoading || !fontManager) { return ( @@ -257,7 +249,6 @@ function BarChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = 'l padding={chartPadding} yKeys={['y']} domainPadding={domainPadding} - onChartBoundsChange={handleChartBoundsChange} onScaleChange={handleScaleChange} renderOutside={renderOutside} xAxis={{ diff --git a/src/components/Charts/LineChart/LineChartContent.tsx b/src/components/Charts/LineChart/LineChartContent.tsx index 1f7a028fb8a1..de64043a9c27 100644 --- a/src/components/Charts/LineChart/LineChartContent.tsx +++ b/src/components/Charts/LineChart/LineChartContent.tsx @@ -16,7 +16,7 @@ import { useDynamicYDomain, useLabelHitTesting, } from '@components/Charts/hooks'; -import {getXAxisLabel, getYAxisLabelWidth, labelOverhang} from '@components/Charts/utils'; +import {getCartesianPlotBounds, getXAxisLabel, getYAxisLabelWidth, labelOverhang} from '@components/Charts/utils'; import VictoryTheme, {getCartesianChartHeight, getXAxisLabelSpace, GLYPH_PADDING, LABEL_PADDING, LABEL_ROTATIONS, SIN_45} from '@components/Charts/VictoryTheme'; import useTheme from '@hooks/useTheme'; @@ -26,9 +26,9 @@ import variables from '@styles/variables'; import CONST from '@src/CONST'; -import type {CartesianChartRenderArg, ChartBounds, Scale} from 'victory-native'; +import type {CartesianChartRenderArg, Scale} from 'victory-native'; -import React, {useState} from 'react'; +import React from 'react'; import {View} from 'react-native'; import {GestureDetector} from 'react-native-gesture-handler'; import Animated, {useAnimatedStyle, useSharedValue} from 'react-native-reanimated'; @@ -54,9 +54,6 @@ function LineChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = ' const theme = useTheme(); const styles = useThemeStyles(); const fontManager = useChartFontManager(); - const [plotAreaWidth, setPlotAreaWidth] = useState(0); - const [boundsLeft, setBoundsLeft] = useState(0); - const [boundsRight, setBoundsRight] = useState(0); const yAxisDomain = useDynamicYDomain(data); const chartData = data.map((point, index) => ({ @@ -87,8 +84,11 @@ function LineChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = ' const yAxisLabelWidth = getYAxisLabelWidth(data, formatValue, fontManager, variables.iconSizeExtraSmall, BASE_DOMAIN_PADDING); - const tickSpacing = plotAreaWidth > 0 && data.length > 0 ? plotAreaWidth / data.length : 0; const chartPaddingLeft = yAxisLabelWidth + GLYPH_PADDING; + const {left: boundsLeft, right: boundsRight} = getCartesianPlotBounds(chartWidth, chartPaddingLeft); + const plotAreaWidth = boundsRight - boundsLeft; + + const tickSpacing = plotAreaWidth > 0 && data.length > 0 ? plotAreaWidth / data.length : 0; const domainPadding = (() => { if (!firstLabelWidth || !lastLabelWidth) { @@ -136,12 +136,6 @@ function LineChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = ' chartBottom, }); - const handleChartBoundsChange = (bounds: ChartBounds) => { - setPlotAreaWidth(bounds.right - bounds.left); - setBoundsLeft(bounds.left); - setBoundsRight(bounds.right); - }; - const checkIsOverDot = (args: HitTestArgs) => { 'worklet'; @@ -252,7 +246,6 @@ function LineChartContentBody({data, isLoading, yAxisUnit, yAxisUnitPosition = ' padding={chartPadding} yKeys={['y']} domainPadding={domainPadding} - onChartBoundsChange={handleChartBoundsChange} onScaleChange={handleScaleChange} renderOutside={renderOutside} xAxis={{ diff --git a/src/components/Charts/utils/index.ts b/src/components/Charts/utils/index.ts index 692b073cc38a..c539de70fcb5 100644 --- a/src/components/Charts/utils/index.ts +++ b/src/components/Charts/utils/index.ts @@ -430,6 +430,20 @@ function getNiceYAxisTicks(rawDataMax: number, rawDataMin: number, tickCount: nu return scaleLinear().domain([paddedMin, paddedMax]).nice().ticks(tickCount); } +/** + * Predicts the horizontal plot bounds victory-native will report for a cartesian chart of this width. + * + * The library sets the x output range to [padding.left + measured y-label width + y labelOffset, width - padding.right] + * and takes the plot bounds from the ends of that range. Our y axis carries no font, so the width it measures for its + * own labels is zero and only labelOffset applies. + */ +function getCartesianPlotBounds(chartWidth: number, chartPaddingLeft: number): {left: number; right: number} { + if (chartWidth <= 0) { + return {left: 0, right: 0}; + } + return {left: chartPaddingLeft + VictoryTheme.axis.labelGap, right: chartWidth - VictoryTheme.axis.padding.right}; +} + /** Returns the pixel width needed for Y-axis labels given the chart data. */ function getYAxisLabelWidth( data: ChartDataPoint[], @@ -476,6 +490,7 @@ export { isCursorInSkewedLabel, isCursorOverChartLabel, getNiceYAxisTicks, + getCartesianPlotBounds, getYAxisLabelWidth, }; From 0cabc99ae9a75218968d7e6a19b79e621e80b335 Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Sat, 26 Sep 2026 16:38:30 +0200 Subject: [PATCH 08/15] fix: measure chart skeleton width before the first paint --- src/components/Charts/ChartSkeleton.tsx | 3 ++- src/components/Charts/ChartWidthBox.tsx | 3 ++- src/hooks/useContainerWidth.ts | 26 +++++++++++++++++++------ 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/components/Charts/ChartSkeleton.tsx b/src/components/Charts/ChartSkeleton.tsx index 48bb85f750c7..82b003196058 100644 --- a/src/components/Charts/ChartSkeleton.tsx +++ b/src/components/Charts/ChartSkeleton.tsx @@ -93,11 +93,12 @@ type ChartSkeletonProps = { function ChartSkeleton({view}: ChartSkeletonProps) { const theme = useTheme(); - const {onLayout, containerWidth} = useContainerWidth(); + const {ref, onLayout, containerWidth} = useContainerWidth(); const {render, height} = PLACEHOLDER_BY_VIEW[view]; return ( diff --git a/src/components/Charts/ChartWidthBox.tsx b/src/components/Charts/ChartWidthBox.tsx index 1c748a5c0ec2..4845fb62cd4e 100644 --- a/src/components/Charts/ChartWidthBox.tsx +++ b/src/components/Charts/ChartWidthBox.tsx @@ -13,10 +13,11 @@ type ChartWidthBoxProps = { /** On web the chart component mounts only once the chart engine has downloaded, so measuring here keeps the width across that mount. */ function ChartWidthBox({children}: ChartWidthBoxProps) { const styles = useThemeStyles(); - const {onLayout, containerWidth} = useContainerWidth(); + const {ref, onLayout, containerWidth} = useContainerWidth(); return ( diff --git a/src/hooks/useContainerWidth.ts b/src/hooks/useContainerWidth.ts index ca2760f5c6d8..fadcc33a7ecc 100644 --- a/src/hooks/useContainerWidth.ts +++ b/src/hooks/useContainerWidth.ts @@ -1,15 +1,29 @@ -import type {LayoutChangeEvent} from 'react-native'; +import isHTMLElement from '@libs/isHTMLElement'; -import {useCallback, useState} from 'react'; +import type {RefObject} from 'react'; +import type {LayoutChangeEvent, View} from 'react-native'; + +import {useCallback, useLayoutEffect, useRef, useState} from 'react'; /** - * Returns an onLayout handler and the measured container width (minus an optional offset). - * Used by skeleton components that need the container width for SVG layout calculations. + * Returns a ref, an onLayout handler and the measured container width (minus an optional offset). * The width updates automatically when the container resizes. + * + * Attach the returned ref to the container as well when its first frame draws from the width: on web, onLayout + * reports only after the browser has painted once, so without the ref that first frame is drawn at width 0. */ -function useContainerWidth(offset = 0): {onLayout: (event: LayoutChangeEvent) => void; containerWidth: number} { +function useContainerWidth(offset = 0): {ref: RefObject; onLayout: (event: LayoutChangeEvent) => void; containerWidth: number} { + const ref = useRef(null); const [containerWidth, setContainerWidth] = useState(0); + useLayoutEffect(() => { + const node: unknown = ref.current; + if (!isHTMLElement(node)) { + return; + } + setContainerWidth(node.getBoundingClientRect().width - offset); + }, [offset]); + const onLayout = useCallback( (event: LayoutChangeEvent) => { setContainerWidth(event.nativeEvent.layout.width - offset); @@ -17,7 +31,7 @@ function useContainerWidth(offset = 0): {onLayout: (event: LayoutChangeEvent) => [offset], ); - return {onLayout, containerWidth}; + return {ref, onLayout, containerWidth}; } export default useContainerWidth; From 35c15a6e4774fd19f52bd132e8c4165c51ba33e0 Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Sat, 26 Sep 2026 16:40:11 +0200 Subject: [PATCH 09/15] feat: keep chart skeleton until the chart first draws on web --- ...kia+2.4.18+004+emit-first-draw-event.patch | 72 +++++++++++++++++++ patches/@shopify/react-native-skia/details.md | 25 +++++++ .../Charts/PieChart/PieChartContent.tsx | 2 +- src/components/Charts/SkiaWebChart/index.tsx | 23 +++--- .../Charts/SkiaWebChart/useHasSkiaDrawn.ts | 24 +++++++ 5 files changed, 137 insertions(+), 9 deletions(-) create mode 100644 patches/@shopify/react-native-skia/@shopify+react-native-skia+2.4.18+004+emit-first-draw-event.patch create mode 100644 src/components/Charts/SkiaWebChart/useHasSkiaDrawn.ts diff --git a/patches/@shopify/react-native-skia/@shopify+react-native-skia+2.4.18+004+emit-first-draw-event.patch b/patches/@shopify/react-native-skia/@shopify+react-native-skia+2.4.18+004+emit-first-draw-event.patch new file mode 100644 index 000000000000..fb4c25d87d63 --- /dev/null +++ b/patches/@shopify/react-native-skia/@shopify+react-native-skia+2.4.18+004+emit-first-draw-event.patch @@ -0,0 +1,72 @@ +diff --git a/node_modules/@shopify/react-native-skia/lib/commonjs/views/SkiaPictureView.web.js b/node_modules/@shopify/react-native-skia/lib/commonjs/views/SkiaPictureView.web.js +index cad87bc..82503a2 100644 +--- a/node_modules/@shopify/react-native-skia/lib/commonjs/views/SkiaPictureView.web.js ++++ b/node_modules/@shopify/react-native-skia/lib/commonjs/views/SkiaPictureView.web.js +@@ -24,6 +24,15 @@ const dp2Pixel = (pd, rect) => { + height: rect.height * pd + }; + }; ++const notifyFirstDraw = renderer => { ++ if (renderer.hasDrawn) { ++ return; ++ } ++ renderer.hasDrawn = true; ++ renderer.canvas.dispatchEvent(new CustomEvent("skia-first-draw", { ++ bubbles: true ++ })); ++}; + class WebGLRenderer { + constructor(canvas, pd) { + this.canvas = canvas; +@@ -95,6 +104,7 @@ class WebGLRenderer { + canvas.drawPicture(picture); + canvas.restore(); + this.surface.ref.flush(); ++ notifyFirstDraw(this); + } + } + dispose() { +@@ -176,6 +186,7 @@ class StaticWebGLRenderer { + // Draw the tempCanvas scaled down to the display size + ctx2d.drawImage(tempCanvas, 0, 0, tempCanvas.width, tempCanvas.height, 0, 0, this.canvas.clientWidth * this.pd, this.canvas.clientHeight * this.pd); + this.cleanupWebGLContext(tempCanvas); ++ notifyFirstDraw(this); + } + makeImageSnapshot(picture, rect) { + if (!this.cachedImage) { +diff --git a/node_modules/@shopify/react-native-skia/lib/module/views/SkiaPictureView.web.js b/node_modules/@shopify/react-native-skia/lib/module/views/SkiaPictureView.web.js +index 12ea0ae..3ee87cd 100644 +--- a/node_modules/@shopify/react-native-skia/lib/module/views/SkiaPictureView.web.js ++++ b/node_modules/@shopify/react-native-skia/lib/module/views/SkiaPictureView.web.js +@@ -18,6 +18,15 @@ const dp2Pixel = (pd, rect) => { + height: rect.height * pd + }; + }; ++const notifyFirstDraw = renderer => { ++ if (renderer.hasDrawn) { ++ return; ++ } ++ renderer.hasDrawn = true; ++ renderer.canvas.dispatchEvent(new CustomEvent("skia-first-draw", { ++ bubbles: true ++ })); ++}; + class WebGLRenderer { + constructor(canvas, pd) { + this.canvas = canvas; +@@ -89,6 +98,7 @@ class WebGLRenderer { + canvas.drawPicture(picture); + canvas.restore(); + this.surface.ref.flush(); ++ notifyFirstDraw(this); + } + } + dispose() { +@@ -170,6 +180,7 @@ class StaticWebGLRenderer { + // Draw the tempCanvas scaled down to the display size + ctx2d.drawImage(tempCanvas, 0, 0, tempCanvas.width, tempCanvas.height, 0, 0, this.canvas.clientWidth * this.pd, this.canvas.clientHeight * this.pd); + this.cleanupWebGLContext(tempCanvas); ++ notifyFirstDraw(this); + } + makeImageSnapshot(picture, rect) { + if (!this.cachedImage) { diff --git a/patches/@shopify/react-native-skia/details.md b/patches/@shopify/react-native-skia/details.md index 5e02782df8f7..a058d9a47cdb 100644 --- a/patches/@shopify/react-native-skia/details.md +++ b/patches/@shopify/react-native-skia/details.md @@ -97,3 +97,28 @@ - Upstream PR/issue: https://github.com/Shopify/react-native-skia/pull/3855 — the same fix, merged upstream on 2026-05-26. Drop this patch once the Skia dependency is bumped to >= 2.6.9. - E/App issue: https://github.com/Expensify/App/issues/98331, https://github.com/Expensify/App/issues/95905 - PR introducing patch: https://github.com/Expensify/App/pull/98437 + +### [@shopify+react-native-skia+2.4.18+004+emit-first-draw-event.patch](@shopify+react-native-skia+2.4.18+004+emit-first-draw-event.patch) + +- Reason: + + ``` + Lets a chart's loading placeholder stay on screen until the chart is actually visible on web, so a + chart goes straight from its skeleton to the drawn chart, with no blank frame in between and no frame + showing both. + + On web, SkiaPictureView creates its renderer only once its canvas reports a layout, and draws the + first picture after that. React Native Web delivers that layout from a ResizeObserver through a + setTimeout, so the canvas is still blank for at least one painted frame after the chart component + mounts, for as long as the browser takes. Skia exposes no public signal for when that first draw has + happened: CanvasProps omits onLayout, onSize changes before the renderer exists, and victory-native + forwards neither to the canvas it renders. + + Fix: after a renderer first draws a picture onto its canvas, it dispatches a bubbling + "skia-first-draw" CustomEvent on that canvas. SkiaWebChart keeps the chart hidden under its + placeholder until the event reaches its container, then shows the chart and removes the placeholder. + ``` + +- Upstream PR/issue: 🛑 TODO. No upstream issue or PR exists yet; Skia has no public API reporting when a web canvas is first drawn, so one has to be proposed. +- E/App issue: 🛑 TODO +- PR introducing patch: https://github.com/Expensify/App/pull/100610 diff --git a/src/components/Charts/PieChart/PieChartContent.tsx b/src/components/Charts/PieChart/PieChartContent.tsx index 8046569ca7b0..0ad64c59105c 100644 --- a/src/components/Charts/PieChart/PieChartContent.tsx +++ b/src/components/Charts/PieChart/PieChartContent.tsx @@ -50,7 +50,7 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice const innerRadius = radius * VictoryTheme.pie.innerRadiusRatio; const pieGeometry = {radius, innerRadius, centerX: chartWidth / 2, centerY: canvasHeight / 2}; - // The chart draws nothing until it has a size, so handing it one lets it draw on the render it mounts in. + // The chart renders no content until it has a size, so handing it one lets it render that content in the same pass it mounts in. const chartSize = chartWidth > 0 ? {width: chartWidth, height: canvasHeight} : undefined; // Slices are sorted by absolute value (largest first) for color assignment, diff --git a/src/components/Charts/SkiaWebChart/index.tsx b/src/components/Charts/SkiaWebChart/index.tsx index 487a3ba5b726..95e410f25ade 100644 --- a/src/components/Charts/SkiaWebChart/index.tsx +++ b/src/components/Charts/SkiaWebChart/index.tsx @@ -14,9 +14,10 @@ import type {ComponentType, ReactNode} from 'react'; import {WithSkiaWeb} from '@shopify/react-native-skia/lib/module/web'; import React, {useRef, useState} from 'react'; -import {View} from 'react-native'; +import {StyleSheet, View} from 'react-native'; import isSkiaWebSupported from './isSkiaWebSupported'; +import useHasSkiaDrawn from './useHasSkiaDrawn'; import useIsSkiaSurfaceUnavailable from './useIsSkiaSurfaceUnavailable'; type SkiaWebChartProps = { @@ -26,7 +27,7 @@ type SkiaWebChartProps = { /** Props forwarded to the lazily-loaded chart component. */ componentProps: TProps; - /** Shown while the chart engine downloads. */ + /** Shown while the chart engine downloads and until the chart first draws. */ loadingFallback?: ReactNode; }; @@ -65,6 +66,8 @@ function SkiaWebChart({getComponent, componentProps, load // The probe can pass while the renderer still ends up without a drawing surface. const isSurfaceUnavailable = useIsSkiaSurfaceUnavailable(containerRef); + const hasDrawn = useHasSkiaDrawn(containerRef); + const isAwaitingFirstDraw = !!loadingFallback && !hasDrawn; if (!isSupported || isSurfaceUnavailable) { return ; @@ -81,12 +84,16 @@ function SkiaWebChart({getComponent, componentProps, load ref={viewRef(containerRef)} style={styles.mw100} > - `/${file}`}} - getComponent={getComponent} - componentProps={componentProps} - fallback={fallback} - /> + {/* The canvas stays blank until Skia draws into it, while the parts of a chart drawn as views show at once. */} + + `/${file}`}} + getComponent={getComponent} + componentProps={componentProps} + fallback={fallback} + /> + + {isAwaitingFirstDraw && {loadingFallback}} ); } diff --git a/src/components/Charts/SkiaWebChart/useHasSkiaDrawn.ts b/src/components/Charts/SkiaWebChart/useHasSkiaDrawn.ts new file mode 100644 index 000000000000..2cfbf75e2dd4 --- /dev/null +++ b/src/components/Charts/SkiaWebChart/useHasSkiaDrawn.ts @@ -0,0 +1,24 @@ +import type {RefObject} from 'react'; + +import {useEffect, useState} from 'react'; + +/** Dispatched by the patched Skia web renderer after it first draws a picture onto its canvas. */ +const FIRST_DRAW_EVENT = 'skia-first-draw'; + +function useHasSkiaDrawn(containerRef: RefObject): boolean { + const [hasDrawn, setHasDrawn] = useState(false); + + useEffect(() => { + const container = containerRef.current; + if (!(container instanceof HTMLElement)) { + return; + } + const markDrawn = () => setHasDrawn(true); + container.addEventListener(FIRST_DRAW_EVENT, markDrawn); + return () => container.removeEventListener(FIRST_DRAW_EVENT, markDrawn); + }, [containerRef]); + + return hasDrawn; +} + +export default useHasSkiaDrawn; From 606538a22c124e4004306695a41f9b4ecc40d2a6 Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Sat, 26 Sep 2026 18:50:27 +0200 Subject: [PATCH 10/15] feat: draw chart skeletons from the design --- src/components/Charts/ChartSkeleton.tsx | 356 +++++++++++++++--- .../Charts/PieChart/PieChartContent.tsx | 6 +- .../components/Charts/ChartSkeleton.test.tsx | 69 +++- 3 files changed, 360 insertions(+), 71 deletions(-) diff --git a/src/components/Charts/ChartSkeleton.tsx b/src/components/Charts/ChartSkeleton.tsx index 82b003196058..5d895ba70cba 100644 --- a/src/components/Charts/ChartSkeleton.tsx +++ b/src/components/Charts/ChartSkeleton.tsx @@ -5,86 +5,342 @@ import SkeletonViewContentLoader from '@components/SkeletonViewContentLoader'; import useContainerWidth from '@hooks/useContainerWidth'; import useTheme from '@hooks/useTheme'; +import variables from '@styles/variables'; + import CONST from '@src/CONST'; -import type {ReactNode} from 'react'; +import type {ReactElement, ReactNode} from 'react'; -import React from 'react'; +import React, {useId} from 'react'; import {View} from 'react-native'; -import {Circle, Path} from 'react-native-svg'; +import {Circle, Defs, LinearGradient, Path, Stop} from 'react-native-svg'; -import {CHART_CONTENT_MIN_HEIGHT, getCartesianChartHeight} from './VictoryTheme'; +import VictoryTheme, {CHART_CONTENT_MIN_HEIGHT, getCartesianChartHeight} from './VictoryTheme'; const CHART_SKELETON_TEST_ID = 'chartSkeleton'; +const BASELINE_TEST_ID = 'chartSkeletonBaseline'; +const Y_AXIS_TEST_ID = 'chartSkeletonYAxis'; const BAR_TEST_ID = 'chartSkeletonBar'; const LINE_TEST_ID = 'chartSkeletonLine'; +const MARKER_TEST_ID = 'chartSkeletonMarker'; const PIE_TEST_ID = 'chartSkeletonPie'; -// Provisional shapes. Design mocks replace them. -const BAR_HEIGHT_RATIOS = [0.45, 0.72, 0.34, 0.9, 0.56, 0.78]; -const BAR_GAP_RATIO = 0.25; -const LINE_POINT_RATIOS = [0.72, 0.44, 0.6, 0.24, 0.38]; -const LINE_THICKNESS = 8; -const PIE_DIAMETER_RATIO = 0.72; +/** Stands in for the y-axis label column, which the loaded chart sizes from its data and the placeholder cannot. */ +const Y_AXIS_GUTTER = 64; -function renderBarShape(width: number) { - const slotWidth = width / BAR_HEIGHT_RATIOS.length; +const AXIS_THICKNESS = 1; - return BAR_HEIGHT_RATIOS.map((ratio, index) => { - const barHeight = CHART_CONTENT_MIN_HEIGHT * ratio; +/** + * CartesianChart lifts the bottom of its y range by twice the y label offset, to make room for x-axis labels of its + * own, even though these charts draw theirs outside the plot. + */ +const Y_RANGE_BOTTOM = CHART_CONTENT_MIN_HEIGHT - VictoryTheme.axis.padding.bottom - VictoryTheme.axis.labelGap * 2; - return ( - - ); - }); +/** + * The y scale's `nice()` rounds the padded domain out to whole steps: one below zero, since the domain starts at zero, + * and about ten above it. So the zero line, which the loaded chart draws as its baseline, sits one step in eleven + * above the bottom of the range. + */ +const Y_RANGE_TYPICAL_STEPS = 11; + +const LINE_SERIES = [0.12, 0.2732, 0.6575, 0.2327, 0.3302, 0.7745, 0.5831, 0.7384, 0.3981, 0.6358, 0.2378, 0.88]; +const LINE_MARKER_RADIUS = 4; +const LINE_HALF_THICKNESS = 1; + +/** The design rings each marker in the card color. */ +const LINE_MARKER_CLEARANCE = LINE_MARKER_RADIUS + 2; + +/** The same fade from the baseline up to the series line that `AreaGradient` gives the loaded chart. */ +const AREA_ALPHA_BASELINE = 0.02; +const AREA_ALPHA_LINE = 0.2; + +const BAR_HEIGHTS = [0.92, 0.68, 0.44, 0.2, 0.11]; +const BAR_WIDTH_RATIO = 0.1431; +const BAR_PITCH_RATIO = 0.1938; +const BAR_INSET_RATIO = 0.0489; +const BAR_BORDER_RADIUS = 8; + +/** Fractions of the ring, largest first, which is the order the loaded chart lays its slices out in. */ +const PIE_SLICE_SHARES = [0.41, 0.214, 0.214, 0.085, 0.077]; + +/** The total and its caption in the middle of the ring. */ +const PIE_LABEL_BARS = [ + {width: 48, offsetY: -17.5}, + {width: 72, offsetY: 6.5}, +]; +const PIE_LABEL_BAR_HEIGHT = 12; + +/** The `mb2` under each item of the pie legend. */ +const PIE_LEGEND_ITEM_MARGIN_BOTTOM = 8; + +/** One row of the legend PieChartContent draws below the ring: the container's top margin, a line of `textNormal` and the item's bottom margin. */ +const PIE_LEGEND_ROW_HEIGHT = variables.qrShareHorizontalPadding + variables.fontSizeNormalHeight + PIE_LEGEND_ITEM_MARGIN_BOTTOM; + +type ChartSkeletonLayers = { + /** Drawn beneath the shimmer in a paint of its own */ + chrome?: ReactElement; + + /** Becomes the shimmer's clip path, so only fill geometry counts and a stroked shape draws nothing */ + shimmer: ReactNode; +}; + +type PlotArea = { + left: number; + top: number; + right: number; + bottom: number; +}; + +type Point = { + x: number; + y: number; +}; + +function getPlotArea(width: number): PlotArea { + const top = VictoryTheme.axis.padding.top; + + return { + left: Y_AXIS_GUTTER, + top, + right: Math.max(Y_AXIS_GUTTER, width - VictoryTheme.axis.padding.right), + bottom: Y_RANGE_BOTTOM - (Y_RANGE_BOTTOM - top) / Y_RANGE_TYPICAL_STEPS, + }; } -function renderLineShape(width: number) { - const step = width / (LINE_POINT_RATIOS.length - 1); - const points = LINE_POINT_RATIOS.map((ratio, index) => ({x: index * step, y: CHART_CONTENT_MIN_HEIGHT * ratio})); +type PlotLineProps = { + plot: PlotArea; +}; - // A stroked polyline would be dropped by the shimmer clip path, which reads fill geometry only. - const topEdge = points.map((point, index) => `${index === 0 ? 'M' : 'L'}${point.x},${point.y}`).join(' '); - const bottomEdge = points - .slice() - .reverse() - .map((point) => `L${point.x},${point.y + LINE_THICKNESS}`) - .join(' '); +const AXIS_HALF_THICKNESS = AXIS_THICKNESS / 2; + +function PlotBaseline({plot}: PlotLineProps) { + const theme = useTheme(); return ( ); } -function renderPieShape(width: number) { - const radius = (Math.min(width, CHART_CONTENT_MIN_HEIGHT) * PIE_DIAMETER_RATIO) / 2; +function PlotYAxis({plot}: PlotLineProps) { + const theme = useTheme(); return ( - ); } +type LineAreaFillProps = { + points: Point[]; + baselineY: number; +}; + +function LineAreaFill({points, baselineY}: LineAreaFillProps) { + const theme = useTheme(); + const gradientId = useId(); + const first = points.at(0); + const last = points.at(-1); + + if (!first || !last) { + return null; + } + + const along = points.map((point) => `L${point.x} ${point.y}`).join(''); + + return ( + <> + + + + + + + + + ); +} + +function buildSeriesPath(points: Point[]): string { + return points + .map((from, index) => { + const to = points.at(index + 1); + if (!to) { + return ''; + } + + const deltaX = to.x - from.x; + const deltaY = to.y - from.y; + const length = Math.hypot(deltaX, deltaY); + + if (length <= LINE_MARKER_CLEARANCE * 2) { + return ''; + } + + const unitX = deltaX / length; + const unitY = deltaY / length; + const startX = from.x + unitX * LINE_MARKER_CLEARANCE; + const startY = from.y + unitY * LINE_MARKER_CLEARANCE; + const endX = to.x - unitX * LINE_MARKER_CLEARANCE; + const endY = to.y - unitY * LINE_MARKER_CLEARANCE; + const offsetX = -unitY * LINE_HALF_THICKNESS; + const offsetY = unitX * LINE_HALF_THICKNESS; + + return `M${startX + offsetX} ${startY + offsetY}L${endX + offsetX} ${endY + offsetY}L${endX - offsetX} ${endY - offsetY}L${startX - offsetX} ${startY - offsetY}Z`; + }) + .join(''); +} + +function renderLineShape(width: number): ChartSkeletonLayers { + const plot = getPlotArea(width); + const points = LINE_SERIES.map((depth, index) => ({ + x: plot.left + ((plot.right - plot.left) * index) / (LINE_SERIES.length - 1), + y: plot.top + (plot.bottom - plot.top) * depth, + })); + + return { + chrome: ( + <> + + + + + ), + shimmer: ( + <> + + {points.map((point, index) => ( + + ))} + + ), + }; +} + +function renderBarShape(width: number): ChartSkeletonLayers { + const plot = getPlotArea(width); + const plotWidth = plot.right - plot.left; + const plotHeight = plot.bottom - plot.top; + + return { + chrome: , + shimmer: BAR_HEIGHTS.map((heightRatio, index) => { + const barHeight = plotHeight * heightRatio; + + return ( + + ); + }), + }; +} + +function toRingPoint(centerX: number, centerY: number, radius: number, angleDegrees: number): Point { + const angle = (angleDegrees * Math.PI) / 180; + return {x: centerX + radius * Math.cos(angle), y: centerY + radius * Math.sin(angle)}; +} + +// Mirrors the geometry PieChartContent hands the loaded chart, including the gap PaddedPieSlice trims from each slice. +function renderPieShape(width: number): ChartSkeletonLayers { + const centerX = width / 2; + const centerY = CHART_CONTENT_MIN_HEIGHT / 2; + const radius = Math.min(width, CHART_CONTENT_MIN_HEIGHT) / 2; + const innerRadius = radius * VictoryTheme.pie.innerRadiusRatio; + const {padAngle, startAngle: ringStartAngle} = VictoryTheme.pie; + + let sliceStartAngle = ringStartAngle; + const slicePaths = PIE_SLICE_SHARES.map((share) => { + const sweep = share * 360; + const startAngle = sliceStartAngle + padAngle / 2; + const endAngle = sliceStartAngle + sweep - padAngle / 2; + sliceStartAngle += sweep; + + const largeArcFlag = endAngle - startAngle > 180 ? 1 : 0; + const outerStart = toRingPoint(centerX, centerY, radius, startAngle); + const outerEnd = toRingPoint(centerX, centerY, radius, endAngle); + const innerEnd = toRingPoint(centerX, centerY, innerRadius, endAngle); + const innerStart = toRingPoint(centerX, centerY, innerRadius, startAngle); + + return ( + `M${outerStart.x} ${outerStart.y}A${radius} ${radius} 0 ${largeArcFlag} 1 ${outerEnd.x} ${outerEnd.y}` + + `L${innerEnd.x} ${innerEnd.y}A${innerRadius} ${innerRadius} 0 ${largeArcFlag} 0 ${innerStart.x} ${innerStart.y}Z` + ); + }); + + return { + shimmer: ( + <> + {slicePaths.map((d, index) => ( + + ))} + {PIE_LABEL_BARS.map((bar, index) => ( + + ))} + + ), + }; +} + // Each height is the box its chart draws into, so the box holds its size when the chart replaces the placeholder. The -// pie chart's legend sits below that box and is not stood in for, so the pie card still grows by the legend. -const PLACEHOLDER_BY_VIEW: Record ReactNode; height: number}> = { +// pie's box is the ring plus one legend row, left empty because the design draws no legend placeholder. +const PLACEHOLDER_BY_VIEW: Record ChartSkeletonLayers; height: number}> = { [CONST.SEARCH.VIEW.BAR]: {render: renderBarShape, height: getCartesianChartHeight()}, [CONST.SEARCH.VIEW.LINE]: {render: renderLineShape, height: getCartesianChartHeight()}, - [CONST.SEARCH.VIEW.PIE]: {render: renderPieShape, height: CHART_CONTENT_MIN_HEIGHT}, + [CONST.SEARCH.VIEW.PIE]: {render: renderPieShape, height: CHART_CONTENT_MIN_HEIGHT + PIE_LEGEND_ROW_HEIGHT}, }; type ChartSkeletonProps = { @@ -95,6 +351,7 @@ function ChartSkeleton({view}: ChartSkeletonProps) { const theme = useTheme(); const {ref, onLayout, containerWidth} = useContainerWidth(); const {render, height} = PLACEHOLDER_BY_VIEW[view]; + const {chrome, shimmer} = render(containerWidth); return ( - {render(containerWidth)} + {shimmer} ); } export default ChartSkeleton; -export {BAR_TEST_ID, CHART_SKELETON_TEST_ID, LINE_TEST_ID, PIE_TEST_ID}; +export {BAR_TEST_ID, BASELINE_TEST_ID, CHART_SKELETON_TEST_ID, LINE_TEST_ID, MARKER_TEST_ID, PIE_TEST_ID, Y_AXIS_TEST_ID}; diff --git a/src/components/Charts/PieChart/PieChartContent.tsx b/src/components/Charts/PieChart/PieChartContent.tsx index 0ad64c59105c..db31a10d899a 100644 --- a/src/components/Charts/PieChart/PieChartContent.tsx +++ b/src/components/Charts/PieChart/PieChartContent.tsx @@ -144,11 +144,7 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice }; if (isLoading) { - return ( - - - - ); + return ; } if (data.length === 0) { diff --git a/tests/unit/components/Charts/ChartSkeleton.test.tsx b/tests/unit/components/Charts/ChartSkeleton.test.tsx index e69c18597f58..4d15516c19c6 100644 --- a/tests/unit/components/Charts/ChartSkeleton.test.tsx +++ b/tests/unit/components/Charts/ChartSkeleton.test.tsx @@ -1,8 +1,9 @@ import {fireEvent, render, screen} from '@testing-library/react-native'; -import ChartSkeleton, {BAR_TEST_ID, CHART_SKELETON_TEST_ID, LINE_TEST_ID, PIE_TEST_ID} from '@components/Charts/ChartSkeleton'; +import ChartSkeleton, {BAR_TEST_ID, BASELINE_TEST_ID, CHART_SKELETON_TEST_ID, LINE_TEST_ID, MARKER_TEST_ID, PIE_TEST_ID, Y_AXIS_TEST_ID} from '@components/Charts/ChartSkeleton'; import {CHART_CONTENT_MIN_HEIGHT, getCartesianChartHeight} from '@components/Charts/VictoryTheme'; import type {ChartView} from '@components/Search/types'; +import SkeletonRect from '@components/SkeletonRect'; import SkeletonViewContentLoader from '@components/SkeletonViewContentLoader'; import CONST from '@src/CONST'; @@ -10,7 +11,7 @@ import CONST from '@src/CONST'; import React from 'react'; const CONTAINER_WIDTH = 320; -const NARROW_CONTAINER_WIDTH = 60; +const NARROW_CONTAINER_WIDTH = 160; // The shapes size off the measured container, which never lays out under the test renderer, so without this every shape would draw at zero width. function renderAtContainerWidth(view: ChartView, width = CONTAINER_WIDTH) { @@ -18,54 +19,88 @@ function renderAtContainerWidth(view: ChartView, width = CONTAINER_WIDTH) { fireEvent(screen.getByTestId(CHART_SKELETON_TEST_ID), 'layout', {nativeEvent: {layout: {width, height: 0}}}); } -function getSkeletonHeight() { - return Number(screen.UNSAFE_getByType(SkeletonViewContentLoader).props.height); +function getLoaderProps() { + return screen.UNSAFE_getByType(SkeletonViewContentLoader).props; +} + +type BarTransform = [{translateX: number}, {translateY: number}]; + +function isBarTransform(transform: unknown): transform is BarTransform { + return Array.isArray(transform) && transform.length === 2; +} + +function getBarBoxes() { + return screen.UNSAFE_getAllByType(SkeletonRect).map(({props}) => { + const transform: unknown = props.transform; + if (!isBarTransform(transform)) { + throw new Error('Bars are positioned by a translateX and a translateY'); + } + const [{translateX}, {translateY}] = transform; + return {left: translateX, right: translateX + Number(props.width), bottom: translateY + Number(props.height)}; + }); } describe('ChartSkeleton', () => { - it('should draw a series of bars for the bar view', () => { + it('should draw bars standing on one baseline with no y-axis for the bar view', () => { renderAtContainerWidth(CONST.SEARCH.VIEW.BAR); - const bars = screen.getAllByTestId(BAR_TEST_ID); + const bars = getBarBoxes(); expect(bars.length).toBeGreaterThan(1); - expect(bars.every((bar) => Number(bar.props.width) > 0 && Number(bar.props.height) > 0)).toBe(true); + expect(bars.every((bar) => bar.right > bar.left)).toBe(true); + expect(new Set(bars.map((bar) => bar.bottom)).size).toBe(1); + expect(screen.getAllByTestId(BAR_TEST_ID)).toHaveLength(bars.length); + expect(getLoaderProps().beforeMask).toBeDefined(); + expect(screen.getByTestId(BASELINE_TEST_ID)).toBeTruthy(); + expect(screen.queryByTestId(Y_AXIS_TEST_ID)).toBeNull(); expect(screen.queryByTestId(LINE_TEST_ID)).toBeNull(); expect(screen.queryByTestId(PIE_TEST_ID)).toBeNull(); }); - it('should keep every bar drawn in a container narrower than the bar series', () => { + it('should keep every bar inside the chart in a narrow container', () => { renderAtContainerWidth(CONST.SEARCH.VIEW.BAR, NARROW_CONTAINER_WIDTH); - expect(screen.getAllByTestId(BAR_TEST_ID).every((bar) => Number(bar.props.width) > 0)).toBe(true); + expect(getBarBoxes().every((bar) => bar.right > bar.left && bar.right <= NARROW_CONTAINER_WIDTH)).toBe(true); }); - it('should draw a polyline band for the line view', () => { + it('should draw the series line as filled segments with a marker at every point for the line view', () => { renderAtContainerWidth(CONST.SEARCH.VIEW.LINE); - expect(String(screen.getByTestId(LINE_TEST_ID).props.d)).toMatch(/^M0,[\d.]+( L[\d.]+,[\d.]+)+ Z$/); + const line = screen.getByTestId(LINE_TEST_ID); + + expect(String(line.props.d)).toMatch(/^(M[\d. -]+(L[\d. -]+){3}Z)+$/); + expect(line.props.stroke).toBeUndefined(); + expect(screen.getAllByTestId(MARKER_TEST_ID).length).toBeGreaterThan(1); + expect(screen.getByTestId(BASELINE_TEST_ID)).toBeTruthy(); + expect(screen.getByTestId(Y_AXIS_TEST_ID)).toBeTruthy(); expect(screen.queryByTestId(BAR_TEST_ID)).toBeNull(); expect(screen.queryByTestId(PIE_TEST_ID)).toBeNull(); }); - it('should draw a disc for the pie view', () => { + it('should draw ring slices with no axes for the pie view', () => { renderAtContainerWidth(CONST.SEARCH.VIEW.PIE); - expect(Number(screen.getByTestId(PIE_TEST_ID).props.r)).toBeGreaterThan(0); + const slices = screen.getAllByTestId(PIE_TEST_ID); + + expect(slices.length).toBeGreaterThan(1); + expect(slices.every((slice) => /A[\d.]+ [\d.]+ 0 [01] 1 .+A[\d.]+ [\d.]+ 0 [01] 0 .+Z$/.test(String(slice.props.d)))).toBe(true); + expect(getLoaderProps().beforeMask).toBeUndefined(); + expect(screen.queryByTestId(BASELINE_TEST_ID)).toBeNull(); + expect(screen.queryByTestId(Y_AXIS_TEST_ID)).toBeNull(); expect(screen.queryByTestId(BAR_TEST_ID)).toBeNull(); expect(screen.queryByTestId(LINE_TEST_ID)).toBeNull(); }); - it('should reserve the x-axis label strip for the cartesian views and not for the pie view', () => { + it('should reserve the x-axis label strip for the cartesian views and a legend row for the pie view', () => { renderAtContainerWidth(CONST.SEARCH.VIEW.LINE); - expect(getSkeletonHeight()).toBe(getCartesianChartHeight()); + expect(Number(getLoaderProps().height)).toBe(getCartesianChartHeight()); screen.unmount(); renderAtContainerWidth(CONST.SEARCH.VIEW.BAR); - expect(getSkeletonHeight()).toBe(getCartesianChartHeight()); + expect(Number(getLoaderProps().height)).toBe(getCartesianChartHeight()); screen.unmount(); renderAtContainerWidth(CONST.SEARCH.VIEW.PIE); - expect(getSkeletonHeight()).toBe(CHART_CONTENT_MIN_HEIGHT); + expect(Number(getLoaderProps().height)).toBeGreaterThan(CHART_CONTENT_MIN_HEIGHT); }); }); From f3193117d3fa13705b8ae52292a8a711d5f903ce Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Sun, 27 Sep 2026 16:55:38 +0200 Subject: [PATCH 11/15] docs: note Skia 2.13.0 still shows a blank first frame --- patches/@shopify/react-native-skia/details.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patches/@shopify/react-native-skia/details.md b/patches/@shopify/react-native-skia/details.md index a058d9a47cdb..10894c222525 100644 --- a/patches/@shopify/react-native-skia/details.md +++ b/patches/@shopify/react-native-skia/details.md @@ -119,6 +119,6 @@ placeholder until the event reaches its container, then shows the chart and removes the placeholder. ``` -- Upstream PR/issue: 🛑 TODO. No upstream issue or PR exists yet; Skia has no public API reporting when a web canvas is first drawn, so one has to be proposed. +- Upstream PR/issue: 🛑 TODO. No upstream issue or PR exists yet; Skia has no public API reporting when a web canvas is first drawn, still true on 2.13.0, so one has to be proposed. Without this patch, 2.13.0 still shows a blank frame between the placeholder and the drawn chart on web (verified manually). - E/App issue: 🛑 TODO - PR introducing patch: https://github.com/Expensify/App/pull/100610 From 737826c32ae29062e381627b39d5e563d31e8fbd Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Mon, 28 Sep 2026 12:45:45 +0200 Subject: [PATCH 12/15] fix: resolve plot bounds probe so knip sees it --- server/victory-chart-renderer/tests/plotBoundsDrift.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/victory-chart-renderer/tests/plotBoundsDrift.test.ts b/server/victory-chart-renderer/tests/plotBoundsDrift.test.ts index fdade39d3482..012a17802d77 100644 --- a/server/victory-chart-renderer/tests/plotBoundsDrift.test.ts +++ b/server/victory-chart-renderer/tests/plotBoundsDrift.test.ts @@ -5,6 +5,7 @@ import {spawnSync} from 'node:child_process'; import {mkdtempSync} from 'node:fs'; import {tmpdir} from 'node:os'; import {basename, join, resolve} from 'node:path'; +import {fileURLToPath} from 'node:url'; import {packageRoot} from './testUtils'; @@ -37,7 +38,7 @@ let probeOutput: ProbeOutput; describe('cartesian plot bounds', () => { beforeAll(async () => { const buildResult = await Bun.build({ - entrypoints: [join(import.meta.dir, 'probes/plotBoundsProbe.tsx')], + entrypoints: [fileURLToPath(import.meta.resolve('./probes/plotBoundsProbe.tsx'))], target: 'bun', packages: 'bundle', conditions: ['react-native'], From 11cd87eacfb6b767006ce6141585bc5463fcbcb6 Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Mon, 28 Sep 2026 16:00:26 +0200 Subject: [PATCH 13/15] fix: drop the pie placeholder's legend row when the legend is hidden --- src/components/Charts/ChartSkeleton.tsx | 26 +++++++------------ .../Charts/PieChart/PieChartContent.tsx | 7 ++++- src/components/Charts/PieChart/index.tsx | 7 ++++- .../components/Charts/ChartSkeleton.test.tsx | 15 ++++++++++- 4 files changed, 36 insertions(+), 19 deletions(-) diff --git a/src/components/Charts/ChartSkeleton.tsx b/src/components/Charts/ChartSkeleton.tsx index 5d895ba70cba..882595b74ea3 100644 --- a/src/components/Charts/ChartSkeleton.tsx +++ b/src/components/Charts/ChartSkeleton.tsx @@ -25,7 +25,6 @@ const LINE_TEST_ID = 'chartSkeletonLine'; const MARKER_TEST_ID = 'chartSkeletonMarker'; const PIE_TEST_ID = 'chartSkeletonPie'; -/** Stands in for the y-axis label column, which the loaded chart sizes from its data and the placeholder cannot. */ const Y_AXIS_GUTTER = 64; const AXIS_THICKNESS = 1; @@ -36,11 +35,7 @@ const AXIS_THICKNESS = 1; */ const Y_RANGE_BOTTOM = CHART_CONTENT_MIN_HEIGHT - VictoryTheme.axis.padding.bottom - VictoryTheme.axis.labelGap * 2; -/** - * The y scale's `nice()` rounds the padded domain out to whole steps: one below zero, since the domain starts at zero, - * and about ten above it. So the zero line, which the loaded chart draws as its baseline, sits one step in eleven - * above the bottom of the range. - */ +/** The y scale's `nice()` rounds the padded domain out to whole steps: one below zero, since the domain starts at zero, and about ten above it. */ const Y_RANGE_TYPICAL_STEPS = 11; const LINE_SERIES = [0.12, 0.2732, 0.6575, 0.2327, 0.3302, 0.7745, 0.5831, 0.7384, 0.3981, 0.6358, 0.2378, 0.88]; @@ -50,7 +45,7 @@ const LINE_HALF_THICKNESS = 1; /** The design rings each marker in the card color. */ const LINE_MARKER_CLEARANCE = LINE_MARKER_RADIUS + 2; -/** The same fade from the baseline up to the series line that `AreaGradient` gives the loaded chart. */ +/** The same fade `AreaGradient` gives the loaded chart. */ const AREA_ALPHA_BASELINE = 0.02; const AREA_ALPHA_LINE = 0.2; @@ -60,24 +55,21 @@ const BAR_PITCH_RATIO = 0.1938; const BAR_INSET_RATIO = 0.0489; const BAR_BORDER_RADIUS = 8; -/** Fractions of the ring, largest first, which is the order the loaded chart lays its slices out in. */ +/** Largest first, which is the order the loaded chart lays its slices out in. */ const PIE_SLICE_SHARES = [0.41, 0.214, 0.214, 0.085, 0.077]; -/** The total and its caption in the middle of the ring. */ const PIE_LABEL_BARS = [ {width: 48, offsetY: -17.5}, {width: 72, offsetY: 6.5}, ]; const PIE_LABEL_BAR_HEIGHT = 12; -/** The `mb2` under each item of the pie legend. */ const PIE_LEGEND_ITEM_MARGIN_BOTTOM = 8; /** One row of the legend PieChartContent draws below the ring: the container's top margin, a line of `textNormal` and the item's bottom margin. */ const PIE_LEGEND_ROW_HEIGHT = variables.qrShareHorizontalPadding + variables.fontSizeNormalHeight + PIE_LEGEND_ITEM_MARGIN_BOTTOM; type ChartSkeletonLayers = { - /** Drawn beneath the shimmer in a paint of its own */ chrome?: ReactElement; /** Becomes the shimmer's clip path, so only fill geometry counts and a stroked shape draws nothing */ @@ -335,22 +327,24 @@ function renderPieShape(width: number): ChartSkeletonLayers { }; } -// Each height is the box its chart draws into, so the box holds its size when the chart replaces the placeholder. The -// pie's box is the ring plus one legend row, left empty because the design draws no legend placeholder. const PLACEHOLDER_BY_VIEW: Record ChartSkeletonLayers; height: number}> = { [CONST.SEARCH.VIEW.BAR]: {render: renderBarShape, height: getCartesianChartHeight()}, [CONST.SEARCH.VIEW.LINE]: {render: renderLineShape, height: getCartesianChartHeight()}, - [CONST.SEARCH.VIEW.PIE]: {render: renderPieShape, height: CHART_CONTENT_MIN_HEIGHT + PIE_LEGEND_ROW_HEIGHT}, + [CONST.SEARCH.VIEW.PIE]: {render: renderPieShape, height: CHART_CONTENT_MIN_HEIGHT}, }; type ChartSkeletonProps = { view: ChartView; + + /** Whether the loaded pie draws its legend. */ + shouldShowLegend?: boolean; }; -function ChartSkeleton({view}: ChartSkeletonProps) { +function ChartSkeleton({view, shouldShowLegend = true}: ChartSkeletonProps) { const theme = useTheme(); const {ref, onLayout, containerWidth} = useContainerWidth(); const {render, height} = PLACEHOLDER_BY_VIEW[view]; + const legendHeight = view === CONST.SEARCH.VIEW.PIE && shouldShowLegend ? PIE_LEGEND_ROW_HEIGHT : 0; const {chrome, shimmer} = render(containerWidth); return ( @@ -361,7 +355,7 @@ function ChartSkeleton({view}: ChartSkeletonProps) { > ; + return ( + + ); } if (data.length === 0) { diff --git a/src/components/Charts/PieChart/index.tsx b/src/components/Charts/PieChart/index.tsx index d8864602aea2..4474395fb37d 100644 --- a/src/components/Charts/PieChart/index.tsx +++ b/src/components/Charts/PieChart/index.tsx @@ -17,7 +17,12 @@ function PieChart(props: PieChartProps) { } + loadingFallback={ + + } /> )} diff --git a/tests/unit/components/Charts/ChartSkeleton.test.tsx b/tests/unit/components/Charts/ChartSkeleton.test.tsx index 4d15516c19c6..11f2a0f4f703 100644 --- a/tests/unit/components/Charts/ChartSkeleton.test.tsx +++ b/tests/unit/components/Charts/ChartSkeleton.test.tsx @@ -13,7 +13,6 @@ import React from 'react'; const CONTAINER_WIDTH = 320; const NARROW_CONTAINER_WIDTH = 160; -// The shapes size off the measured container, which never lays out under the test renderer, so without this every shape would draw at zero width. function renderAtContainerWidth(view: ChartView, width = CONTAINER_WIDTH) { render(); fireEvent(screen.getByTestId(CHART_SKELETON_TEST_ID), 'layout', {nativeEvent: {layout: {width, height: 0}}}); @@ -103,4 +102,18 @@ describe('ChartSkeleton', () => { renderAtContainerWidth(CONST.SEARCH.VIEW.PIE); expect(Number(getLoaderProps().height)).toBeGreaterThan(CHART_CONTENT_MIN_HEIGHT); }); + + it('should reserve no legend row for a pie that hides its legend', () => { + // Given a pie whose legend is replaced by a table below it + // When its placeholder renders + render( + , + ); + + // Then the box is only the ring, so the table does not move up when the pie replaces the placeholder + expect(Number(getLoaderProps().height)).toBe(CHART_CONTENT_MIN_HEIGHT); + }); }); From 25d246a6c35cc284b1a6b94844e766ba66abcfe3 Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Mon, 28 Sep 2026 16:02:20 +0200 Subject: [PATCH 14/15] chore: restore chart comments removed by a sweep --- .../Charts/LineChart/LineChartContent.tsx | 3 ++ .../Charts/PieChart/PieChartContent.tsx | 10 +++++- src/components/Charts/SkiaWebChart/index.tsx | 9 ++++-- src/components/Charts/VictoryTheme.ts | 31 ++++++++++++++++--- 4 files changed, 45 insertions(+), 8 deletions(-) diff --git a/src/components/Charts/LineChart/LineChartContent.tsx b/src/components/Charts/LineChart/LineChartContent.tsx index 4590c02a2089..ee40b6c8547f 100644 --- a/src/components/Charts/LineChart/LineChartContent.tsx +++ b/src/components/Charts/LineChart/LineChartContent.tsx @@ -36,10 +36,13 @@ import {CartesianChart, Line} from 'victory-native'; import type {CartesianChartProps, ChartDataPoint} from '..'; +/** Inner dot radius for line chart data points */ const DOT_RADIUS = 4; +/** Extra hover area beyond the dot radius for easier touch targeting */ const DOT_HOVER_EXTRA_RADIUS = 2; +/** Base domain padding applied to all sides */ const BASE_DOMAIN_PADDING = {top: 16, bottom: 16, left: 0, right: 0}; type LineChartProps = CartesianChartProps & { diff --git a/src/components/Charts/PieChart/PieChartContent.tsx b/src/components/Charts/PieChart/PieChartContent.tsx index 475b6d9cf137..a7dfbcc13772 100644 --- a/src/components/Charts/PieChart/PieChartContent.tsx +++ b/src/components/Charts/PieChart/PieChartContent.tsx @@ -27,7 +27,7 @@ type PieChartProps = ChartProps & { /** Symbol/unit for value labels in tooltip (e.g., '$', '€'). */ valueUnit?: string; - /** Defaults to 'left'. */ + /** Position of the unit symbol relative to the value. Defaults to 'left'. */ valueUnitPosition?: UnitPosition; /** Whether to draw the slice legend below the donut */ @@ -45,11 +45,13 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice const [activeSliceIndex, setActiveSliceIndex] = useState(-1); const [isHoveringOverPie, setIsHoveringOverPie] = useState(false); + // Shared values for hover state const isHovering = useSharedValue(false); const cursorX = useSharedValue(0); const cursorY = useSharedValue(0); const tooltipPosition = useSharedValue({x: 0, y: 0}); + // Calculate pie geometry const canvasHeight = CHART_CONTENT_MIN_HEIGHT; const radius = Math.min(chartWidth, canvasHeight) / 2; const innerRadius = radius * VictoryTheme.pie.innerRadiusRatio; @@ -67,6 +69,7 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice const {formatValue} = useChartLabelFormats({data, unit: valueUnit, unitPosition: valueUnitPosition}); const tooltipData = useTooltipData(activeOriginalDataIndex, data, formatValue); + // Handle hover state updates const updateActiveSlice = (x: number, y: number) => { const {centerX, centerY} = pieGeometry; const sliceIndex = findSliceAtPosition(x, y, centerX, centerY, radius, innerRadius, processedSlices); @@ -74,6 +77,7 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice setIsHoveringOverPie(sliceIndex >= 0); }; + // Handle slice press callback const handleSlicePress = (sliceIndex: number) => { if (sliceIndex < 0 || sliceIndex >= processedSlices.length) { return; @@ -88,6 +92,7 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice } }; + // Hover gesture const hoverGesture = () => Gesture.Hover() .onBegin((e) => { @@ -115,6 +120,7 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice scheduleOnRN(setIsHoveringOverPie, false); }); + // Tap gesture for click/tap navigation const tapGesture = () => Gesture.Tap().onEnd((e) => { 'worklet'; @@ -127,6 +133,7 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice } }); + // Combined gestures - Race allows both hover and tap to work independently const combinedGesture = Gesture.Race(hoverGesture(), tapGesture()); const renderLegendItem = (slice: PieSlice) => { @@ -206,6 +213,7 @@ function PieChartContent({data, isLoading, valueUnit, valueUnitPosition, onSlice )} + {/* Tooltip */} {activeSliceIndex >= 0 && !!tooltipData && ( ` would reject the // interface-based render-html renderer props (VictoryChartRendererProps) that lack an index signature. @@ -64,11 +65,13 @@ function SkiaWebChart({getComponent, componentProps, load // while a fresh chart still re-checks capability instead of trusting a stale session-wide result. const [isSupported] = useState(() => isSkiaWebSupported()); - // The probe can pass while the renderer still ends up without a drawing surface. + // The probe can pass while the renderer still ends up without a drawing surface, so also listen for the + // renderer reporting that and degrade to the empty state. const isSurfaceUnavailable = useIsSkiaSurfaceUnavailable(containerRef); const hasDrawn = useHasSkiaDrawn(containerRef); const isAwaitingFirstDraw = !!loadingFallback && !hasDrawn; + // If unsupported, the device can't give CanvasKit a usable WebGL surface. if (!isSupported || isSurfaceUnavailable) { return ; } diff --git a/src/components/Charts/VictoryTheme.ts b/src/components/Charts/VictoryTheme.ts index 7c397e171dd8..76d99e1b5aad 100644 --- a/src/components/Charts/VictoryTheme.ts +++ b/src/components/Charts/VictoryTheme.ts @@ -1,16 +1,29 @@ +/** + * Centralized styles and layout constants for the chart components. + */ import colors from '@styles/theme/colors'; import variables from '@styles/variables'; import {CHART_FONT_FAMILY_NAMES} from './utils/chartFontConstants'; -/** The shade order maximizes contrast between adjacent palette entries, keeping adjacent chart segments easy to distinguish. */ +/** Shade groups in the palette */ const CHART_PALETTE_SHADES = [400, 600, 300, 500, 700] as const; +/** Hues cycling within each shade group */ const CHART_PALETTE_HUES = ['yellow', 'tangerine', 'pink', 'green', 'ice', 'blue'] as const; +/** + * Expensify Chart Color Palette. + * + * Shades are ordered (400, 600, 300, 500, 700) so that sequential colors have + * maximum contrast, making adjacent chart segments easy to distinguish. + * + * Within each shade, hues cycle: Yellow, Tangerine, Pink, Green, Ice, Blue. + */ const CHART_PALETTE: string[] = (() => { const palette: string[] = []; + // Generate the 30 unique combinations (5 shades × 6 hues) for (const shade of CHART_PALETTE_SHADES) { for (const hue of CHART_PALETTE_HUES) { const colorKey = `${hue}${shade}`; @@ -23,9 +36,13 @@ const CHART_PALETTE: string[] = (() => { return palette; })(); +/** + * Gets a color from the chart palette based on index. + * Automatically loops back to the start if the index exceeds 29. + */ function getChartColor(index: number): string { if (CHART_PALETTE.length === 0) { - return colors.black; + return colors.black; // Fallback } return CHART_PALETTE.at(index % CHART_PALETTE.length) ?? colors.black; } @@ -39,7 +56,9 @@ const DEFAULT_CHART_DOT_COLOR_INDEX = DEFAULT_CHART_COLOR_INDEX + CHART_PALETTE_ const VictoryTheme = { colors: { palette: CHART_PALETTE, + /** Default color used for single-color charts (e.g., line chart, single-color bar chart) */ default: getChartColor(DEFAULT_CHART_COLOR_INDEX), + /** Default dot color for line chart data points, one shade darker than the line */ defaultDot: getChartColor(DEFAULT_CHART_DOT_COLOR_INDEX), getColor: getChartColor, }, @@ -47,15 +66,19 @@ const VictoryTheme = { axis: { /** Number of Y-axis ticks (including zero) */ tickCount: 5, + /** Line width for X-axis (hidden) */ xLineWidth: 0, /** Line width for Y-axis grid lines */ yLineWidth: 1, - /** Visual gap (px) between axis labels and the chart edge */ + /** Desired visual gap (px) between axis labels and the chart edge, used for both axes */ labelGap: 12, + /** Base chart padding applied to all sides */ padding: {top: 5, left: 5, right: 5, bottom: 5}, }, tooltip: { + /** The height of the chart tooltip pointer */ pointerHeight: 4, + /** The width of the chart tooltip pointer */ pointerWidth: 12, }, pie: { @@ -88,7 +111,7 @@ function getCartesianChartHeight(xAxisLabelHeight = 0): number { return CHART_CONTENT_MIN_HEIGHT + getXAxisLabelSpace(xAxisLabelHeight); } -/** Pixel height of the y-scale output range inside CartesianChart. */ +/** Pixel height of the y-scale output range inside CartesianChart (content height minus base axis padding). */ const CHART_Y_SCALE_HEIGHT = CHART_CONTENT_MIN_HEIGHT - VictoryTheme.axis.padding.top - VictoryTheme.axis.padding.bottom; /** Supported label rotation angles in degrees */ From caaee893b9e4bddd550b52ecbb84d20d3f3180fa Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Tue, 29 Sep 2026 17:36:31 +0200 Subject: [PATCH 15/15] chore: restore three ChartSkeleton comments --- src/components/Charts/ChartSkeleton.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/components/Charts/ChartSkeleton.tsx b/src/components/Charts/ChartSkeleton.tsx index 882595b74ea3..350e80d2c796 100644 --- a/src/components/Charts/ChartSkeleton.tsx +++ b/src/components/Charts/ChartSkeleton.tsx @@ -25,6 +25,7 @@ const LINE_TEST_ID = 'chartSkeletonLine'; const MARKER_TEST_ID = 'chartSkeletonMarker'; const PIE_TEST_ID = 'chartSkeletonPie'; +/** Stands in for the y-axis label column, which the loaded chart sizes from its data and the placeholder cannot. */ const Y_AXIS_GUTTER = 64; const AXIS_THICKNESS = 1; @@ -70,6 +71,7 @@ const PIE_LEGEND_ITEM_MARGIN_BOTTOM = 8; const PIE_LEGEND_ROW_HEIGHT = variables.qrShareHorizontalPadding + variables.fontSizeNormalHeight + PIE_LEGEND_ITEM_MARGIN_BOTTOM; type ChartSkeletonLayers = { + /** Drawn beneath the shimmer in a paint of its own */ chrome?: ReactElement; /** Becomes the shimmer's clip path, so only fill geometry counts and a stroked shape draws nothing */ @@ -327,6 +329,7 @@ function renderPieShape(width: number): ChartSkeletonLayers { }; } +// Each height is the box its chart draws into, so the box holds its size when the chart replaces the placeholder. const PLACEHOLDER_BY_VIEW: Record ChartSkeletonLayers; height: number}> = { [CONST.SEARCH.VIEW.BAR]: {render: renderBarShape, height: getCartesianChartHeight()}, [CONST.SEARCH.VIEW.LINE]: {render: renderLineShape, height: getCartesianChartHeight()}, @@ -344,6 +347,7 @@ function ChartSkeleton({view, shouldShowLegend = true}: ChartSkeletonProps) { const theme = useTheme(); const {ref, onLayout, containerWidth} = useContainerWidth(); const {render, height} = PLACEHOLDER_BY_VIEW[view]; + // The legend row is left empty, since the design draws no legend placeholder. const legendHeight = view === CONST.SEARCH.VIEW.PIE && shouldShowLegend ? PIE_LEGEND_ROW_HEIGHT : 0; const {chrome, shimmer} = render(containerWidth);