diff --git a/jest/setup.ts b/jest/setup.ts index d7e3eec8321e..04a37bb6295c 100644 --- a/jest/setup.ts +++ b/jest/setup.ts @@ -365,6 +365,8 @@ 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, ), + 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/server/libs/bundleWithRnStubs.ts b/server/libs/bundleWithRnStubs.ts new file mode 100644 index 000000000000..448daf25f504 --- /dev/null +++ b/server/libs/bundleWithRnStubs.ts @@ -0,0 +1,33 @@ +import assertBuildSuccess from '@server/libs/assertBuildSuccess'; +import createRnStubPlugin from '@server/plugins/rnStubPlugin'; +import {basename, dirname, join, resolve} from 'node:path'; + +type BundleWithRnStubsOptions = { + packageRoot: string; + entrypoint: string; + outFile: string; +}; + +/** Every output is written beside outFile, because the bundle loads CanvasKit's wasm from beside itself. */ +async function bundleWithRnStubs({packageRoot, entrypoint, outFile}: BundleWithRnStubsOptions): Promise { + const buildResult = await Bun.build({ + entrypoints: [entrypoint], + target: 'bun', + packages: 'bundle', + conditions: ['react-native'], + tsconfig: join(packageRoot, 'tsconfig.json'), + plugins: [createRnStubPlugin(resolve(packageRoot, '../stubs'))], + }); + + assertBuildSuccess(buildResult, `Failed to bundle ${entrypoint}`); + + if (buildResult.outputs.length === 0) { + throw new Error(`Bundled output of ${entrypoint} is missing`); + } + + for (const output of buildResult.outputs) { + await Bun.write(output.kind === 'entry-point' ? outFile : join(dirname(outFile), basename(output.path)), output); + } +} + +export default bundleWithRnStubs; diff --git a/server/victory-chart-renderer/scripts/dev.ts b/server/victory-chart-renderer/scripts/dev.ts index 28e91ab32ffd..f71b01ec13a8 100644 --- a/server/victory-chart-renderer/scripts/dev.ts +++ b/server/victory-chart-renderer/scripts/dev.ts @@ -2,52 +2,25 @@ * Development runner for the victory-chart-renderer CLI. * * victory-native is hoisted to the App repo root, so a bare `bun run src/cli.tsx` would - * resolve real react-native native sources that Bun cannot bundle. This script bundles - * src/cli.tsx with rnStubPlugin (redirecting RN imports to server/stubs), writes the - * bundle to .dev/cli.js, then executes it under the current Bun binary. + * resolve real react-native native sources that Bun cannot bundle. * * process.argv entries after this script are forwarded unchanged to the bundled CLI. * The child process cwd is the App repository root (two levels above this package). * * For a standalone executable instead of a Bun-run bundle, use build.ts. */ -import assertBuildSuccess from '@server/libs/assertBuildSuccess'; -import createRnStubPlugin from '@server/plugins/rnStubPlugin'; +import bundleWithRnStubs from '@server/libs/bundleWithRnStubs'; import {spawnSync} from 'node:child_process'; import {mkdirSync} from 'node:fs'; -import {basename, join, resolve} from 'node:path'; +import {join, resolve} from 'node:path'; const packageRoot = resolve(import.meta.dir, '..'); const repoRoot = resolve(packageRoot, '../..'); -const stubRoot = resolve(packageRoot, '../stubs'); -const tsconfigPath = join(packageRoot, 'tsconfig.json'); const outFile = resolve(packageRoot, '.dev/cli.js'); mkdirSync(join(packageRoot, '.dev'), {recursive: true}); -const buildResult = await Bun.build({ - entrypoints: [resolve(packageRoot, 'src/bootstrap.tsx')], - target: 'bun', - packages: 'bundle', - conditions: ['react-native'], - tsconfig: tsconfigPath, - plugins: [createRnStubPlugin(stubRoot)], -}); - -assertBuildSuccess(buildResult, 'Failed to bundle victory-chart-renderer CLI'); - -if (buildResult.outputs.length === 0) { - throw new Error('Bundled CLI output is missing'); -} - -const devDir = join(packageRoot, '.dev'); - -for (const output of buildResult.outputs) { - const outputName = basename(output.path); - const destName = output.kind === 'entry-point' ? 'cli.js' : outputName; - - await Bun.write(join(devDir, destName), output); -} +await bundleWithRnStubs({packageRoot, entrypoint: resolve(packageRoot, 'src/bootstrap.tsx'), outFile}); const runResult = spawnSync(process.execPath, [outFile, ...process.argv.slice(2)], { cwd: repoRoot, 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..7a889732edd3 --- /dev/null +++ b/server/victory-chart-renderer/tests/plotBoundsDrift.test.ts @@ -0,0 +1,70 @@ +import {afterAll, beforeAll, describe, expect, test} from 'bun:test'; + +import type {ChartBounds} from 'victory-native'; + +import bundleWithRnStubs from '@server/libs/bundleWithRnStubs'; +import {spawnSync} from 'node:child_process'; +import {mkdtempSync, rmSync} from 'node:fs'; +import {tmpdir} from 'node:os'; +import {join, resolve} from 'node:path'; +import {fileURLToPath} from 'node:url'; + +import {packageRoot} from './testUtils'; + +type PlotBounds = Pick; + +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 () => { + await bundleWithRnStubs({ + packageRoot, + entrypoint: fileURLToPath(import.meta.resolve('./probes/plotBoundsProbe.tsx')), + outFile: bundlePath, + }); + + 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); + + afterAll(() => { + rmSync(runDir, {recursive: true, force: true}); + }); + + test('should predict the plot bounds victory-native reports', () => { + // Given a real chart laid out the way the vertical bar chart is, whose padding and axes the line chart shares, drawn headless by the probe + // When its reported plot bounds are compared with the ones predicted from its width + // Then they match, so a victory-native upgrade that moves the plot fails here instead of misplacing labels + 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..f4fd7087213d --- /dev/null +++ b/server/victory-chart-renderer/tests/probes/plotBoundsProbe.tsx @@ -0,0 +1,9 @@ +/* + * 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..7fc9af2e003c --- /dev/null +++ b/server/victory-chart-renderer/tests/probes/plotBoundsProbeMain.tsx @@ -0,0 +1,63 @@ +import {VERTICAL_BAR_DOMAIN_PADDING} from '@components/Charts/barChartConstants'; +import {getCartesianPlotBounds} from '@components/Charts/utils'; +import VictoryTheme, {CARTESIAN_Y_AXIS, CHART_CONTENT_MIN_HEIGHT, GLYPH_PADDING} from '@components/Charts/VictoryTheme'; + +import type {ChartBounds} from 'victory-native'; + +import {drawOffscreen, makeOffscreenSurface} from '@shopify/react-native-skia/lib/module/headless'; +import {CartesianChart} from 'victory-native'; + +const CHART_WIDTH = 400; + +const Y_AXIS_LABEL_WIDTH = 37; + +const CHART_PADDING_RIGHT = Y_AXIS_LABEL_WIDTH + GLYPH_PADDING; + +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_CONTENT_MIN_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 {left, right} = getCartesianPlotBounds(CHART_WIDTH, CHART_PADDING_RIGHT); + +console.log(JSON.stringify({predicted: {left, right}, 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 04112b6a66b5..bd489db5b24f 100644 --- a/src/components/Charts/BarChart/BarChartContent.tsx +++ b/src/components/Charts/BarChart/BarChartContent.tsx @@ -2,11 +2,11 @@ import {ChartFontsProvider} from '@components/Charts/hooks'; import React from 'react'; -import type BarChartProps from './types'; +import type {BarChartContentProps} from './types'; import VerticalBarChartContentBody from './VerticalBarChartContent'; -function BarChartContent(props: BarChartProps) { +function BarChartContent(props: BarChartContentProps) { return ( diff --git a/src/components/Charts/BarChart/VerticalBarChartContent.tsx b/src/components/Charts/BarChart/VerticalBarChartContent.tsx index b4ed45026b78..77f584ccec54 100644 --- a/src/components/Charts/BarChart/VerticalBarChartContent.tsx +++ b/src/components/Charts/BarChart/VerticalBarChartContent.tsx @@ -4,43 +4,41 @@ import ChartGridLines from '@components/Charts/components/ChartGridLines'; import ChartTooltipLayer from '@components/Charts/components/ChartTooltipLayer'; import ChartXAxisLabels from '@components/Charts/components/ChartXAxisLabels'; import ChartYAxisLabels from '@components/Charts/components/ChartYAxisLabels'; -import type {HitTestArgs} from '@components/Charts/hooks'; +import type {CartesianCanvasInputs, HitTestArgs} from '@components/Charts/hooks'; import { - useChartFontManager, useChartInteractions, useChartLabelFormats, useChartLabelLayout, useChartLabelMeasurements, useDynamicYDomain, useLabelHitTesting, + useReadyCartesianCanvas, } from '@components/Charts/hooks'; -import {getBarLayout, getPrimaryValue, getXAxisLabel, getYAxisLabelWidth} from '@components/Charts/utils'; -import VictoryTheme, {CHART_CONTENT_MIN_HEIGHT, GLYPH_PADDING} from '@components/Charts/VictoryTheme'; +import {getBarLayout, getCartesianPlotBounds, getPrimaryValue, getXAxisLabel, getYAxisLabelWidth} from '@components/Charts/utils'; +import VictoryTheme, {CARTESIAN_Y_AXIS, CHART_CONTENT_MIN_HEIGHT, GLYPH_PADDING} from '@components/Charts/VictoryTheme'; +import useStyleUtils from '@hooks/useStyleUtils'; import useTheme from '@hooks/useTheme'; import useThemeStyles from '@hooks/useThemeStyles'; import variables from '@styles/variables'; -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'; import type BarChartProps from './types'; +import type {BarChartContentProps} from './types'; -function VerticalBarChartContentBody({data, series, isLoading, yAxisUnit, yAxisUnitPosition = 'left', onBarPress, shouldShowLabels = true}: BarChartProps) { +type VerticalBarChartCanvasProps = Omit & CartesianCanvasInputs; + +function VerticalBarChartCanvas({data, series, yAxisUnit, yAxisUnitPosition = 'left', onBarPress, shouldShowLabels = true, chartWidth, fontManager}: VerticalBarChartCanvasProps) { const theme = useTheme(); const styles = useThemeStyles(); - const fontManager = useChartFontManager(); - const [chartWidth, setChartWidth] = useState(0); - const [barAreaWidth, setBarAreaWidth] = useState(0); - const [boundsLeft, setBoundsLeft] = useState(0); - const [boundsRight, setBoundsRight] = useState(0); + const StyleUtils = useStyleUtils(); const [{key: primarySeriesKey}] = series; const chartData = data.map((point, index) => ({ @@ -60,16 +58,21 @@ function VerticalBarChartContentBody({data, series, isLoading, yAxisUnit, yAxisU } }; - const handleLayout = (event: LayoutChangeEvent) => { - setChartWidth(event.nativeEvent.layout.width); - }; - - const barLayout = getBarLayout(barAreaWidth, data.length); - // Empty label data makes the measurement and layout hooks return early instead of laying out labels nobody sees. const labelData = shouldShowLabels ? data : []; const originalLabels = labelData.map(getXAxisLabel); + const {formatValue, formatCompactValue} = useChartLabelFormats({ + data, + unit: yAxisUnit, + unitPosition: yAxisUnitPosition, + }); + + const yAxisLabelWidth = getYAxisLabelWidth(data, formatCompactValue, fontManager, variables.iconSizeExtraSmall, VERTICAL_BAR_DOMAIN_PADDING); + const chartPaddingRight = yAxisLabelWidth + GLYPH_PADDING + VictoryTheme.axis.labelGap; + const plotBounds = getCartesianPlotBounds(chartWidth, chartPaddingRight); + const barLayout = getBarLayout(plotBounds.width, data.length); + const measurements = useChartLabelMeasurements(labelData, fontManager, variables.iconSizeExtraSmall); const {labelRotation, labelSkipInterval, truncatedLabelWidths, xAxisLabelHeight, regularLabelMaxWidth, firstLabelMaxWidth, lastLabelMaxWidth, ellipsisWidth} = useChartLabelLayout({ @@ -77,16 +80,10 @@ function VerticalBarChartContentBody({data, series, isLoading, yAxisUnit, yAxisU fontManager, fontSize: variables.iconSizeExtraSmall, measurements, - tickSpacing: shouldShowLabels && barAreaWidth > 0 ? barLayout.barWidth + barLayout.gap : 0, - labelAreaWidth: barAreaWidth, - firstTickLeftSpace: boundsLeft + barLayout.edgeSpace, - lastTickRightSpace: chartWidth > 0 ? chartWidth - boundsRight + barLayout.edgeSpace : 0, - }); - - const {formatValue, formatCompactValue} = useChartLabelFormats({ - data, - unit: yAxisUnit, - unitPosition: yAxisUnitPosition, + tickSpacing: shouldShowLabels && plotBounds.width > 0 ? barLayout.barWidth + barLayout.gap : 0, + labelAreaWidth: plotBounds.width, + firstTickLeftSpace: plotBounds.left + barLayout.edgeSpace, + lastTickRightSpace: chartWidth - plotBounds.right + barLayout.edgeSpace, }); const barHitHalfWidth = useSharedValue(0); @@ -105,15 +102,9 @@ function VerticalBarChartContentBody({data, series, isLoading, yAxisUnit, yAxisU }); const handleChartBoundsChange = (bounds: ChartBounds) => { - const domainWidth = bounds.right - bounds.left; - const {barWidth, gap} = getBarLayout(domainWidth, data.length); - barHitHalfWidth.set(barWidth > 0 ? barWidth / 2 + gap * BAR_HIT_GAP_RATIO : 0); plotTop.set(bounds.top); plotBottom.set(bounds.bottom); yZero.set(0); - setBarAreaWidth(domainWidth); - setBoundsLeft(bounds.left); - setBoundsRight(bounds.right); }; const checkIsOverBar = (args: HitTestArgs) => { @@ -139,6 +130,7 @@ function VerticalBarChartContentBody({data, series, isLoading, yAxisUnit, yAxisU }); const handleScaleChange = (xScale: Scale, yScale: Scale) => { + barHitHalfWidth.set(barLayout.barWidth > 0 ? barLayout.barWidth / 2 + barLayout.gap * BAR_HIT_GAP_RATIO : 0); yZero.set(yScale(0)); updateTickPositions(xScale, data.length); setPointPositions( @@ -169,7 +161,7 @@ function VerticalBarChartContentBody({data, series, isLoading, yAxisUnit, yAxisU }; const renderOutside = (args: CartesianChartRenderArg<{x: number; y: number}, 'y'>) => { - if (!fontManager || xAxisLabelHeight === undefined) { + if (xAxisLabelHeight === undefined) { return null; } @@ -209,21 +201,8 @@ function VerticalBarChartContentBody({data, series, isLoading, yAxisUnit, yAxisU }; const labelSpace = shouldShowLabels ? VictoryTheme.axis.xAxisLabelGap + (xAxisLabelHeight ?? 0) : 0; - const dynamicChartStyle = {height: CHART_CONTENT_MIN_HEIGHT + labelSpace}; - const yAxisLabelWidth = getYAxisLabelWidth(data, formatCompactValue, fontManager, variables.iconSizeExtraSmall, VERTICAL_BAR_DOMAIN_PADDING); - const chartPadding = {...VictoryTheme.axis.padding, bottom: labelSpace + VictoryTheme.axis.padding.bottom, right: yAxisLabelWidth + GLYPH_PADDING + VictoryTheme.axis.labelGap}; - - if (isLoading || !fontManager) { - return ( - - - - ); - } - - if (data.length === 0) { - return null; - } + const chartHeight = CHART_CONTENT_MIN_HEIGHT + labelSpace; + const chartPadding = {...VictoryTheme.axis.padding, bottom: labelSpace + VictoryTheme.axis.padding.bottom, right: chartPaddingRight}; return ( - {chartWidth > 0 && ( - - {({points, chartBounds, yScale, yTicks}) => ( - <> - - {points.y.map((point) => renderBar(point, chartBounds))} - - )} - - )} + + {({points, chartBounds, yScale, yTicks}) => ( + <> + + {points.y.map((point) => renderBar(point, chartBounds))} + + )} + + + + ); + } + + return ( + + ); +} + export default VerticalBarChartContentBody; diff --git a/src/components/Charts/BarChart/index.native.tsx b/src/components/Charts/BarChart/index.native.tsx index 63798b3dc21b..9aea26a2ab8a 100644 --- a/src/components/Charts/BarChart/index.native.tsx +++ b/src/components/Charts/BarChart/index.native.tsx @@ -1,3 +1,5 @@ +import ChartWidthBox from '@components/Charts/components/ChartWidthBox'; + import React from 'react'; import type BarChartProps from './types'; @@ -5,7 +7,16 @@ import type BarChartProps from './types'; import BarChartContent from './BarChartContent'; function BarChart(props: BarChartProps) { - return ; + return ( + + {(chartWidth) => ( + + )} + + ); } export default BarChart; diff --git a/src/components/Charts/BarChart/index.tsx b/src/components/Charts/BarChart/index.tsx index 50d908c8083e..e3b165f0a3b5 100644 --- a/src/components/Charts/BarChart/index.tsx +++ b/src/components/Charts/BarChart/index.tsx @@ -1,3 +1,4 @@ +import ChartWidthBox from '@components/Charts/components/ChartWidthBox'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; import React from 'react'; @@ -7,10 +8,14 @@ import type BarChartProps from './types'; const getBarChartContent = () => import('./BarChartContent'); function BarChart(props: BarChartProps) { return ( - + + {(chartWidth) => ( + + )} + ); } diff --git a/src/components/Charts/BarChart/types.ts b/src/components/Charts/BarChart/types.ts index 064e1322dc25..88eee9d07669 100644 --- a/src/components/Charts/BarChart/types.ts +++ b/src/components/Charts/BarChart/types.ts @@ -7,4 +7,10 @@ type BarChartProps = CartesianChartProps & { shouldShowLabels?: boolean; }; +type BarChartContentProps = BarChartProps & { + /** Null until the card has been measured. */ + chartWidth: number | null; +}; + export default BarChartProps; +export type {BarChartContentProps}; diff --git a/src/components/Charts/LineChart/LineChartContent.tsx b/src/components/Charts/LineChart/LineChartContent.tsx index 644c44cc198d..4f3be4254ea1 100644 --- a/src/components/Charts/LineChart/LineChartContent.tsx +++ b/src/components/Charts/LineChart/LineChartContent.tsx @@ -5,30 +5,38 @@ import ChartGridLines from '@components/Charts/components/ChartGridLines'; import ChartTooltipLayer from '@components/Charts/components/ChartTooltipLayer'; import ChartXAxisLabels from '@components/Charts/components/ChartXAxisLabels'; import ChartYAxisLabels from '@components/Charts/components/ChartYAxisLabels'; -import type {HitTestArgs} from '@components/Charts/hooks'; +import type {CartesianCanvasInputs, HitTestArgs} from '@components/Charts/hooks'; import { ChartFontsProvider, - useChartFontManager, useChartInteractions, useChartLabelFormats, useChartLabelLayout, useChartLabelMeasurements, useDynamicYDomain, useLabelHitTesting, + useReadyCartesianCanvas, } from '@components/Charts/hooks'; -import {getAxisLabelStep, getDomainPaddingForEdgeSpace, getPrimaryValue, getXAxisLabel, getYAxisLabelWidth, labelOverhang} from '@components/Charts/utils'; -import VictoryTheme, {CHART_CONTENT_MIN_HEIGHT, DASH_INTERVALS, GLYPH_PADDING, LABEL_PADDING, LABEL_ROTATIONS, SIN_45, X_AXIS_LABEL_MIN_GAP} from '@components/Charts/VictoryTheme'; - +import {getAxisLabelStep, getCartesianPlotBounds, getDomainPaddingForEdgeSpace, getPrimaryValue, getXAxisLabel, getYAxisLabelWidth, labelOverhang} from '@components/Charts/utils'; +import VictoryTheme, { + CARTESIAN_Y_AXIS, + CHART_CONTENT_MIN_HEIGHT, + DASH_INTERVALS, + GLYPH_PADDING, + LABEL_PADDING, + LABEL_ROTATIONS, + SIN_45, + X_AXIS_LABEL_MIN_GAP, +} from '@components/Charts/VictoryTheme'; + +import useStyleUtils from '@hooks/useStyleUtils'; import useTheme from '@hooks/useTheme'; import useThemeStyles from '@hooks/useThemeStyles'; import variables from '@styles/variables'; -import type {LayoutChangeEvent} from 'react-native'; import type {CartesianChartRenderArg, ChartBounds, Scale} from 'victory-native'; import {DashPathEffect} from '@shopify/react-native-skia'; -import React, {useState} from 'react'; import {View} from 'react-native'; import {GestureDetector} from 'react-native-gesture-handler'; import Animated, {useAnimatedStyle, useDerivedValue, useSharedValue} from 'react-native-reanimated'; @@ -43,14 +51,17 @@ type LineChartProps = CartesianChartProps & { labelSteps?: readonly number[]; }; -function LineChartContentBody({data, series, isLoading, yAxisUnit, yAxisUnitPosition = 'left', onPointPress, labelSteps}: LineChartProps) { +type LineChartContentProps = LineChartProps & { + /** Null until the card has been measured. */ + chartWidth: number | null; +}; + +type LineChartCanvasProps = Omit & CartesianCanvasInputs; + +function LineChartCanvas({data, series, yAxisUnit, yAxisUnitPosition = 'left', onPointPress, labelSteps, chartWidth, fontManager}: LineChartCanvasProps) { const theme = useTheme(); const styles = useThemeStyles(); - const fontManager = useChartFontManager(); - const [chartWidth, setChartWidth] = useState(0); - const [plotAreaWidth, setPlotAreaWidth] = useState(0); - const [boundsLeft, setBoundsLeft] = useState(0); - const [boundsRight, setBoundsRight] = useState(0); + const StyleUtils = useStyleUtils(); const yAxisDomain = useDynamicYDomain(data); const isLastPointInProgress = !!data.at(-1)?.isInProgress; @@ -73,10 +84,6 @@ function LineChartContentBody({data, series, isLoading, yAxisUnit, yAxisUnitPosi } }; - const handleLayout = (event: LayoutChangeEvent) => { - setChartWidth(event.nativeEvent.layout.width); - }; - const chartBottom = useSharedValue(0); const plotTop = useSharedValue(0); const plotLeft = useSharedValue(0); @@ -94,8 +101,9 @@ function LineChartContentBody({data, series, isLoading, yAxisUnit, yAxisUnitPosi const yAxisLabelWidth = getYAxisLabelWidth(data, formatCompactValue, fontManager, variables.iconSizeExtraSmall, VictoryTheme.line.domainPadding); - const tickSpacing = plotAreaWidth > 0 && data.length > 0 ? plotAreaWidth / data.length : 0; const chartPaddingRight = yAxisLabelWidth + GLYPH_PADDING + VictoryTheme.axis.labelGap; + const {left: boundsLeft, right: boundsRight, width: plotAreaWidth} = getCartesianPlotBounds(chartWidth, chartPaddingRight); + const tickSpacing = plotAreaWidth > 0 && data.length > 0 ? plotAreaWidth / data.length : 0; const domainPadding = (() => { if (!firstLabelWidth || !lastLabelWidth) { @@ -129,12 +137,12 @@ function LineChartContentBody({data, series, isLoading, yAxisUnit, yAxisUnitPosi // Stepped labels never rotate, so a zero width makes this hook return early instead of computing a rotation labelAreaWidth: labelSteps ? 0 : plotAreaWidth, firstTickLeftSpace: boundsLeft + domainPadding.left * paddingScale, - lastTickRightSpace: chartWidth > 0 ? chartWidth - boundsRight + domainPadding.right * paddingScale : 0, + lastTickRightSpace: chartWidth - boundsRight + domainPadding.right * paddingScale, measurements, }); const steppedLabelLayout = (() => { - if (!labelSteps || chartWidth <= 0 || plotAreaWidth <= 0) { + if (!labelSteps || plotAreaWidth <= 0) { return undefined; } const firstPointX = boundsLeft + domainPadding.left * paddingScale; @@ -171,9 +179,6 @@ function LineChartContentBody({data, series, isLoading, yAxisUnit, yAxisUnitPosi plotTop.set(bounds.top); plotLeft.set(bounds.left); plotRight.set(bounds.right); - setPlotAreaWidth(bounds.right - bounds.left); - setBoundsLeft(bounds.left); - setBoundsRight(bounds.right); }; const labelSpace = VictoryTheme.axis.xAxisLabelGap + (xAxisLabelHeight ?? 0); @@ -262,7 +267,7 @@ function LineChartContentBody({data, series, isLoading, yAxisUnit, yAxisUnitPosi )} - {xAxisLabelHeight !== undefined && !!fontManager && ( + {xAxisLabelHeight !== undefined && ( )} - {!!fontManager && ( - - )} + - - - ); - } - - if (data.length === 0) { - return null; - } - return ( - {chartWidth > 0 && ( - - {({yScale, yTicks, chartBounds}) => ( - - )} - - )} + + {({yScale, yTicks, chartBounds}) => ( + + )} + + + + ); + } + + return ( + + ); +} + +function LineChartContent(props: LineChartContentProps) { return ( diff --git a/src/components/Charts/LineChart/index.native.tsx b/src/components/Charts/LineChart/index.native.tsx index 2c6ac7c2941f..2fd8ea813de1 100644 --- a/src/components/Charts/LineChart/index.native.tsx +++ b/src/components/Charts/LineChart/index.native.tsx @@ -1,3 +1,5 @@ +import ChartWidthBox from '@components/Charts/components/ChartWidthBox'; + import React from 'react'; import type {LineChartProps} from './LineChartContent'; @@ -5,7 +7,16 @@ import type {LineChartProps} from './LineChartContent'; import LineChartContent from './LineChartContent'; function LineChart(props: LineChartProps) { - return ; + return ( + + {(chartWidth) => ( + + )} + + ); } LineChart.displayName = 'LineChart'; diff --git a/src/components/Charts/LineChart/index.tsx b/src/components/Charts/LineChart/index.tsx index 7de73f7fcf44..01d3da19af53 100644 --- a/src/components/Charts/LineChart/index.tsx +++ b/src/components/Charts/LineChart/index.tsx @@ -1,3 +1,4 @@ +import ChartWidthBox from '@components/Charts/components/ChartWidthBox'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; import React from 'react'; @@ -7,10 +8,14 @@ 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 aa3ef07cd178..691f22d55310 100644 --- a/src/components/Charts/PieChart/PieChartContent.tsx +++ b/src/components/Charts/PieChart/PieChartContent.tsx @@ -3,15 +3,13 @@ import ChartTooltipLayer from '@components/Charts/components/ChartTooltipLayer'; import {TOOLTIP_BAR_GAP, useChartLabelFormats, useTouchPinnedTooltip} from '@components/Charts/hooks'; import type {ChartDataPoint, ChartProps, PieSlice, UnitPosition} from '@components/Charts/types'; import {findSliceAtPosition, getPrimaryValue, 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'; import useStyleUtils from '@hooks/useStyleUtils'; import useThemeStyles from '@hooks/useThemeStyles'; -import type {LayoutChangeEvent} from 'react-native'; - import React, {useState} from 'react'; import {View} from 'react-native'; import {Gesture, GestureDetector, PointerType} from 'react-native-gesture-handler'; @@ -34,12 +32,19 @@ type PieChartProps = ChartProps & { shouldShowLegend?: boolean; }; -function PieChartContent({data, series, isLoading, valueUnit, valueUnitPosition, onSlicePress, shouldShowLegend = true}: PieChartProps) { +type PieChartContentProps = PieChartProps & { + /** Null until the card has been measured. */ + chartWidth: number | null; +}; + +type PieChartCanvasProps = Omit & { + chartWidth: number; +}; + +function PieChartCanvas({data, series, valueUnit, valueUnitPosition, onSlicePress, shouldShowLegend = true, chartWidth}: PieChartCanvasProps) { const styles = useThemeStyles(); const StyleUtils = useStyleUtils(); const {translate} = useLocalize(); - const [canvasWidth, setCanvasWidth] = useState(0); - const [canvasHeight, setCanvasHeight] = useState(0); const [isHoveringOverPie, setIsHoveringOverPie] = useState(false); // Shared values for hover state @@ -52,15 +57,10 @@ function PieChartContent({data, series, isLoading, valueUnit, valueUnitPosition, const matchedIndex = useSharedValue(-1); const isTooltipActive = useDerivedValue(() => matchedIndex.get() >= 0); - const handleLayout = (event: LayoutChangeEvent) => { - setCanvasWidth(event.nativeEvent.layout.width); - setCanvasHeight(event.nativeEvent.layout.height); - }; - - // 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}; // 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 @@ -201,18 +201,6 @@ function PieChartContent({data, series, isLoading, valueUnit, valueUnitPosition, ); }; - if (isLoading) { - return ( - - - - ); - } - - if (data.length === 0) { - return null; - } - return ( <> {processedSlices.length > 0 && ( + + + ); + } + + return ( + + ); +} + export default PieChartContent; export type {PieChartProps}; diff --git a/src/components/Charts/PieChart/index.native.tsx b/src/components/Charts/PieChart/index.native.tsx index 961f5bdfb82c..97d224a27cbf 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/components/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 66999fee714f..1441e2cdc847 100644 --- a/src/components/Charts/PieChart/index.tsx +++ b/src/components/Charts/PieChart/index.tsx @@ -1,3 +1,4 @@ +import ChartWidthBox from '@components/Charts/components/ChartWidthBox'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; import React from 'react'; @@ -8,10 +9,14 @@ const getPieChartContent = () => import('./PieChartContent'); function PieChart(props: PieChartProps) { return ( - + + {(chartWidth) => ( + + )} + ); } diff --git a/src/components/Charts/VictoryTheme.ts b/src/components/Charts/VictoryTheme.ts index 4adb3259df3f..2af60fdd94ad 100644 --- a/src/components/Charts/VictoryTheme.ts +++ b/src/components/Charts/VictoryTheme.ts @@ -91,6 +91,12 @@ const VictoryTheme = { }, } as const; +const CARTESIAN_Y_AXIS = { + tickCount: VictoryTheme.axis.tickCount, + axisSide: 'right', + lineWidth: 0, +} as const; + /** Minimum height for the chart content area (bars, Y-axis, grid lines) */ const CHART_CONTENT_MIN_HEIGHT = 250; @@ -134,6 +140,7 @@ const DASH_INTERVALS = [4, 8]; const GLYPH_PADDING = 4; export { + CARTESIAN_Y_AXIS, CHART_CONTENT_MIN_HEIGHT, X_AXIS_LABEL_MIN_GAP, CHART_Y_SCALE_HEIGHT, diff --git a/src/components/Charts/components/ChartWidthBox.tsx b/src/components/Charts/components/ChartWidthBox.tsx new file mode 100644 index 000000000000..4d71efe2c303 --- /dev/null +++ b/src/components/Charts/components/ChartWidthBox.tsx @@ -0,0 +1,39 @@ +import useThemeStyles from '@hooks/useThemeStyles'; + +import type {ReactNode} from 'react'; +import type {LayoutChangeEvent} from 'react-native'; + +import {useState} from 'react'; +import {View} from 'react-native'; + +type ChartWidthBoxProps = { + /** Receives the card's width, or null until the card has been measured. */ + children: (chartWidth: number | null) => 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 [chartWidth, setChartWidth] = useState(null); + + const onLayout = (event: LayoutChangeEvent) => { + const {width} = event.nativeEvent.layout; + + // A hidden card reports a width of 0. Keeping the last width keeps the chart and its hover state mounted until the card shows again. + if (width <= 0) { + return; + } + setChartWidth(width); + }; + + return ( + + {children(chartWidth)} + + ); +} + +export default ChartWidthBox; diff --git a/src/components/Charts/hooks/index.ts b/src/components/Charts/hooks/index.ts index 63f9204d5dde..496999231eb1 100644 --- a/src/components/Charts/hooks/index.ts +++ b/src/components/Charts/hooks/index.ts @@ -1,7 +1,6 @@ export {default as useChartLabelLayout} from './useChartLabelLayout'; export {default as useChartLabelMeasurements} from './useChartLabelMeasurements'; export {default as useChartParagraphs} from './useChartParagraphs'; -export {useChartFontManager} from '@components/Charts/context/ChartFontsContext'; export {default as ChartFontsProvider} from '@components/Charts/context/ChartFontsProvider'; export {useChartInteractions, TOOLTIP_BAR_GAP} from './useChartInteractions'; export type {HitTestArgs, ResolveTargetIndexArgs} from './useChartInteractions'; @@ -9,4 +8,6 @@ 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 useReadyCartesianCanvas} from './useReadyCartesianCanvas'; +export type {CartesianCanvasInputs} from './useReadyCartesianCanvas'; export {default as useTouchPinnedTooltip} from './useTouchPinnedTooltip'; diff --git a/src/components/Charts/hooks/useReadyCartesianCanvas.ts b/src/components/Charts/hooks/useReadyCartesianCanvas.ts new file mode 100644 index 000000000000..13e7d3288bfa --- /dev/null +++ b/src/components/Charts/hooks/useReadyCartesianCanvas.ts @@ -0,0 +1,21 @@ +import {useChartFontManager} from '@components/Charts/context/ChartFontsContext'; + +import type {SkTypefaceFontProvider} from '@shopify/react-native-skia'; + +type CartesianCanvasInputs = { + chartWidth: number; + fontManager: SkTypefaceFontProvider; +}; + +function useReadyCartesianCanvas(isLoading: boolean, chartWidth: number | null): CartesianCanvasInputs | null { + const fontManager = useChartFontManager(); + + if (isLoading || !fontManager || chartWidth === null) { + return null; + } + + return {chartWidth, fontManager}; +} + +export default useReadyCartesianCanvas; +export type {CartesianCanvasInputs}; diff --git a/src/components/Charts/utils/index.ts b/src/components/Charts/utils/index.ts index e2fc0e38b37a..890f3a0e61a8 100644 --- a/src/components/Charts/utils/index.ts +++ b/src/components/Charts/utils/index.ts @@ -455,6 +455,16 @@ function getBarLayout(plotWidth: number, barCount: number): {barWidth: number; g return {barWidth, gap, edgeSpace, xDomain: [-edgeSpaceInXUnits, barCount - 1 + edgeSpaceInXUnits]}; } +/** + * Horizontal plot bounds of a cartesian chart whose y axis is `CARTESIAN_Y_AXIS`, mirroring victory-native's layout, + * so labels can be laid out from the width before the chart mounts. + */ +function getCartesianPlotBounds(chartWidth: number, paddingRight: number): {left: number; right: number; width: number} { + const left = VictoryTheme.axis.padding.left; + const right = Math.max(left, chartWidth - paddingRight); + return {left, right, width: right - left}; +} + /** * Domain padding that leaves `edgeSpace` px between the plot edges and the first and last points. * victory-native fits the padded domain back into the plot width, so the space on screen is smaller than the padding. @@ -552,6 +562,7 @@ export { isCursorOverChartLabel, getNiceYAxisTicks, getYAxisLabelWidth, + getCartesianPlotBounds, getBarLayout, getDomainPaddingForEdgeSpace, getAxisLabelStep, diff --git a/tests/unit/components/Charts/chartWidthHandoff.test.tsx b/tests/unit/components/Charts/chartWidthHandoff.test.tsx new file mode 100644 index 000000000000..b34aefe53c3e --- /dev/null +++ b/tests/unit/components/Charts/chartWidthHandoff.test.tsx @@ -0,0 +1,121 @@ +import {fireEvent, render, screen} from '@testing-library/react-native'; + +import ActivityIndicator from '@components/ActivityIndicator'; +import BarChart from '@components/Charts/BarChart'; +import LineChart from '@components/Charts/LineChart'; +import PieChart from '@components/Charts/PieChart'; +import type {ChartDataPoint, ChartProps} from '@components/Charts/types'; +import {CHART_CONTENT_MIN_HEIGHT} from '@components/Charts/VictoryTheme'; +import {CHART_SERIES_KEY} from '@components/Search/buildChartSeries'; + +import React from 'react'; +import {PolarChart} from 'victory-native'; + +const CONTAINER_WIDTH = 320; + +const data: ChartDataPoint[] = [ + {label: 'Amazon', values: {[CHART_SERIES_KEY.PRIMARY]: 120}}, + {label: 'Uber', values: {[CHART_SERIES_KEY.PRIMARY]: 80}}, +]; + +const series: ChartProps['series'] = [{key: CHART_SERIES_KEY.PRIMARY}]; + +function getMeasuredBox() { + let node = screen.UNSAFE_getByType(ActivityIndicator).parent; + + while (node) { + if (typeof node.props.onLayout === 'function') { + return node; + } + node = node.parent; + } + + throw new Error('the loading spinner has no ancestor reporting its layout'); +} + +describe('chart width handoff', () => { + afterEach(() => { + jest.clearAllMocks(); + }); + + it.each([ + ['bar', BarChart], + ['line', LineChart], + ])('should measure the %s chart width while the chart is still loading', (_name, Chart) => { + // Given a chart whose data has not arrived yet + const loadingChart = ( + + ); + + // When it renders its loading state + render(loadingChart); + + // Then the spinner already sits inside the box that measures the width, + // so the width is known by the time the data arrives and the chart can be sized in the pass it mounts in + expect(getMeasuredBox()).toBeTruthy(); + }); + + it('should hand the pie chart the width measured while it was loading', () => { + // Given a loading pie chart whose box has been measured + const {rerender} = render( + , + ); + fireEvent(getMeasuredBox(), 'layout', {nativeEvent: {layout: {width: CONTAINER_WIDTH, height: CHART_CONTENT_MIN_HEIGHT}}}); + + // When the data arrives + rerender( + , + ); + + // Then the chart mounts with that size instead of measuring itself, which would cost it a render with no content + expect(PolarChart).toHaveBeenCalledWith(expect.objectContaining({explicitSize: {width: CONTAINER_WIDTH, height: CHART_CONTENT_MIN_HEIGHT}}), undefined); + }); + + it('should keep the pie chart under the spinner until a width has been measured', () => { + // Given a pie chart whose box has not reported a layout yet + // When it renders with its data + render( + , + ); + + // Then the spinner stays and the chart does not mount, because it can only be sized once the width is known + expect(PolarChart).not.toHaveBeenCalled(); + expect(screen.UNSAFE_queryByType(ActivityIndicator)).not.toBeNull(); + }); + + it('should keep the pie chart mounted when its box reports a width of 0', () => { + // Given a pie chart that has mounted at its measured width + render( + , + ); + const measuredBox = getMeasuredBox(); + fireEvent(measuredBox, 'layout', {nativeEvent: {layout: {width: CONTAINER_WIDTH, height: CHART_CONTENT_MIN_HEIGHT}}}); + + // When its card is hidden and the box reports a width of 0 + fireEvent(measuredBox, 'layout', {nativeEvent: {layout: {width: 0, height: 0}}}); + + // Then the chart stays mounted at its last width instead of falling back to the spinner, which would drop its hover state and rebuild its canvas + expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); + expect(PolarChart).toHaveBeenLastCalledWith(expect.objectContaining({explicitSize: {width: CONTAINER_WIDTH, height: CHART_CONTENT_MIN_HEIGHT}}), undefined); + }); +}); diff --git a/tests/unit/components/Charts/utils.test.ts b/tests/unit/components/Charts/utils.test.ts index 5850ae539046..a7e0ff3c83d5 100644 --- a/tests/unit/components/Charts/utils.test.ts +++ b/tests/unit/components/Charts/utils.test.ts @@ -11,6 +11,7 @@ import { getAdditionalOffset, getDomainPaddingForEdgeSpace, getNiceYAxisTicks, + getCartesianPlotBounds, isAngleInSlice, isCursorInSkewedLabel, isCursorOverChartLabel, @@ -754,6 +755,29 @@ describe('getBarLayout', () => { }); }); +describe('getCartesianPlotBounds', () => { + const PADDING_LEFT = VictoryTheme.axis.padding.left; + + it('reserves the right gutter for labels and the left base padding', () => { + // Given a 300px container with a 30px right gutter + // When computing the plot bounds + // Then the plot spans from the left padding to the gutter, where victory-native puts it, + // so labels laid out before the chart mounts line up with the plot it draws + expect(getCartesianPlotBounds(300, 30)).toEqual({left: PADDING_LEFT, right: 300 - 30, width: 300 - 30 - PADDING_LEFT}); + }); + + it('clamps to a zero-width plot when the container is too small for the gutters', () => { + // Given a container narrower than the right gutter itself + // When computing the plot bounds + // Then the plot collapses to zero width at the left padding, because a negative width would break the bar and + // label layouts that are sized from it + const bounds = getCartesianPlotBounds(10, 30); + expect(bounds.left).toBe(PADDING_LEFT); + expect(bounds.right).toBe(PADDING_LEFT); + expect(bounds.width).toBe(0); + }); +}); + describe('getDomainPaddingForEdgeSpace', () => { it('returns the padding victory-native shrinks back to the requested edge space', () => { // Given a 200px plot and the space wanted before the first and after the last point