diff --git a/src/components/Charts/BarChart/VerticalBarChartContent.tsx b/src/components/Charts/BarChart/VerticalBarChartContent.tsx index 78f0b3fa08d6..9da40b5a3b59 100644 --- a/src/components/Charts/BarChart/VerticalBarChartContent.tsx +++ b/src/components/Charts/BarChart/VerticalBarChartContent.tsx @@ -1,4 +1,3 @@ -import ActivityIndicator from '@components/ActivityIndicator'; import {BAR_CORNER_RADIUS, BAR_GROUP_INNER_GAP, BAR_HIT_GAP_RATIO, VERTICAL_BAR_DOMAIN_PADDING} from '@components/Charts/barChartConstants'; import ChartGridLines from '@components/Charts/components/ChartGridLines'; import ChartTooltipLayer from '@components/Charts/components/ChartTooltipLayer'; @@ -17,7 +16,8 @@ import { } from '@components/Charts/hooks'; import type {ChartSeries, WrappedLabelConfig} from '@components/Charts/types'; import {getBarLayout, getCartesianPlotBounds, getXAxisLabel, getYAxisLabelWidth} from '@components/Charts/utils'; -import VictoryTheme, {CARTESIAN_Y_AXIS, CHART_CONTENT_MIN_HEIGHT, GLYPH_PADDING, MAX_BAR_LABEL_LINES} from '@components/Charts/VictoryTheme'; +import {getBarChartHeight, getXAxisLabelSpace} from '@components/Charts/utils/chartHeights'; +import VictoryTheme, {CARTESIAN_Y_AXIS, GLYPH_PADDING, MAX_BAR_LABEL_LINES} from '@components/Charts/VictoryTheme'; import useStyleUtils from '@hooks/useStyleUtils'; import useTheme from '@hooks/useTheme'; @@ -28,7 +28,6 @@ import variables from '@styles/variables'; import type {TupleToUnion} from 'type-fest'; import type {CartesianChartRenderArg, ChartBounds, PointsArray, Scale} from 'victory-native'; -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'; @@ -115,6 +114,7 @@ function VerticalBarChartCanvas({ canvasWidth: chartWidth, } : undefined; + // `getBarChartLoadingHeight` reserves the same lines while the chart loads, so update the two together. const wrapLabelHeight = wrapConfig ? wrapConfig.maxLines * wrapConfig.lineHeight : 0; // When wrapping, labelAreaWidth is 0 above so the hook returns EMPTY_LAYOUT (horizontal, skip 1). // xAxisLabelHeight is 0 there too, so the wrap height has to be substituted explicitly. @@ -242,8 +242,8 @@ function VerticalBarChartCanvas({ ); }; - const labelSpace = shouldShowLabels ? VictoryTheme.axis.xAxisLabelGap + resolvedLabelHeight : 0; - const chartHeight = CHART_CONTENT_MIN_HEIGHT + labelSpace; + const labelSpace = shouldShowLabels ? getXAxisLabelSpace(resolvedLabelHeight) : 0; + const chartHeight = getBarChartHeight(shouldShowLabels, resolvedLabelHeight); const chartPadding = {...VictoryTheme.axis.padding, bottom: labelSpace + VictoryTheme.axis.padding.bottom, right: chartPaddingRight}; return ( @@ -306,19 +306,10 @@ function VerticalBarChartCanvas({ } function VerticalBarChartContentBody({isLoading = false, chartWidth, ...canvasProps}: BarChartContentProps) { - const styles = useThemeStyles(); const readyCanvas = useReadyCartesianCanvas(isLoading, chartWidth); - if (!isLoading && canvasProps.data.length === 0) { - return null; - } - if (!readyCanvas) { - return ( - - - - ); + return null; } return ( diff --git a/src/components/Charts/BarChart/index.native.tsx b/src/components/Charts/BarChart/index.native.tsx index 9aea26a2ab8a..32233f3380b7 100644 --- a/src/components/Charts/BarChart/index.native.tsx +++ b/src/components/Charts/BarChart/index.native.tsx @@ -1,4 +1,5 @@ -import ChartWidthBox from '@components/Charts/components/ChartWidthBox'; +import ChartFrame from '@components/Charts/components/ChartFrame'; +import {getBarChartLoadingHeight} from '@components/Charts/utils/chartHeights'; import React from 'react'; @@ -8,14 +9,18 @@ import BarChartContent from './BarChartContent'; function BarChart(props: BarChartProps) { return ( - + 0} + getLoadingHeight={() => getBarChartLoadingHeight(props.shouldShowLabels, props.shouldWrapLabels)} + > {(chartWidth) => ( )} - + ); } diff --git a/src/components/Charts/BarChart/index.tsx b/src/components/Charts/BarChart/index.tsx index e3b165f0a3b5..cbfcc883f846 100644 --- a/src/components/Charts/BarChart/index.tsx +++ b/src/components/Charts/BarChart/index.tsx @@ -1,21 +1,27 @@ -import ChartWidthBox from '@components/Charts/components/ChartWidthBox'; +import ChartFrame from '@components/Charts/components/ChartFrame'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; +import {getBarChartLoadingHeight} from '@components/Charts/utils/chartHeights'; import React from 'react'; import type BarChartProps from './types'; const getBarChartContent = () => import('./BarChartContent'); + function BarChart(props: BarChartProps) { return ( - + 0} + getLoadingHeight={() => getBarChartLoadingHeight(props.shouldShowLabels, props.shouldWrapLabels)} + > {(chartWidth) => ( )} - + ); } diff --git a/src/components/Charts/LineChart/LineChartContent.tsx b/src/components/Charts/LineChart/LineChartContent.tsx index 41574ddd6771..7b03e3b5f85f 100644 --- a/src/components/Charts/LineChart/LineChartContent.tsx +++ b/src/components/Charts/LineChart/LineChartContent.tsx @@ -1,4 +1,3 @@ -import ActivityIndicator from '@components/ActivityIndicator'; import ActivePointIndicator from '@components/Charts/components/ActivePointIndicator'; import AreaGradient from '@components/Charts/components/AreaGradient'; import ChartGridLines from '@components/Charts/components/ChartGridLines'; @@ -18,17 +17,9 @@ import { useReadyCartesianCanvas, } from '@components/Charts/hooks'; import {getAxisLabelStep, getCartesianPlotBounds, getDomainPaddingForEdgeSpace, getLineRuns, getXAxisLabel, getYAxisLabelWidth, labelOverhang} from '@components/Charts/utils'; +import {getCartesianChartHeight, getXAxisLabelSpace} from '@components/Charts/utils/chartHeights'; import type {LineRun} from '@components/Charts/utils/getLineRuns'; -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 VictoryTheme, {CARTESIAN_Y_AXIS, 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'; @@ -41,7 +32,6 @@ import type {CartesianChartRenderArg, ChartBounds, Scale} from 'victory-native'; import {DashPathEffect} from '@shopify/react-native-skia'; import React from 'react'; -import {View} from 'react-native'; import {GestureDetector} from 'react-native-gesture-handler'; import Animated, {useAnimatedStyle, useDerivedValue, useSharedValue} from 'react-native-reanimated'; import {CartesianChart, Line} from 'victory-native'; @@ -186,7 +176,7 @@ function LineChartCanvas({data, series, yAxisUnit, yAxisUnitPosition = 'left', o plotRight.set(bounds.right); }; - const labelSpace = VictoryTheme.axis.xAxisLabelGap + (xAxisLabelHeight ?? 0); + const labelSpace = getXAxisLabelSpace(xAxisLabelHeight); const isInPlotArea = (args: HitTestArgs) => { 'worklet'; @@ -332,7 +322,7 @@ function LineChartCanvas({data, series, yAxisUnit, yAxisUnitPosition = 'left', o ); }; - const chartHeight = CHART_CONTENT_MIN_HEIGHT + labelSpace; + const chartHeight = getCartesianChartHeight(xAxisLabelHeight); const chartPadding = { ...VictoryTheme.axis.padding, bottom: labelSpace + VictoryTheme.axis.padding.bottom, @@ -394,19 +384,10 @@ function LineChartCanvas({data, series, yAxisUnit, yAxisUnitPosition = 'left', o } function LineChartContentBody({isLoading = false, chartWidth, ...canvasProps}: LineChartContentProps) { - const styles = useThemeStyles(); const readyCanvas = useReadyCartesianCanvas(isLoading, chartWidth); - if (!isLoading && canvasProps.data.length === 0) { - return null; - } - if (!readyCanvas) { - return ( - - - - ); + return null; } return ( diff --git a/src/components/Charts/LineChart/index.native.tsx b/src/components/Charts/LineChart/index.native.tsx index 2fd8ea813de1..1ec454bbfae8 100644 --- a/src/components/Charts/LineChart/index.native.tsx +++ b/src/components/Charts/LineChart/index.native.tsx @@ -1,4 +1,5 @@ -import ChartWidthBox from '@components/Charts/components/ChartWidthBox'; +import ChartFrame from '@components/Charts/components/ChartFrame'; +import {getCartesianChartHeight} from '@components/Charts/utils/chartHeights'; import React from 'react'; @@ -8,14 +9,18 @@ import LineChartContent from './LineChartContent'; function LineChart(props: LineChartProps) { return ( - + 0} + getLoadingHeight={() => getCartesianChartHeight()} + > {(chartWidth) => ( )} - + ); } diff --git a/src/components/Charts/LineChart/index.tsx b/src/components/Charts/LineChart/index.tsx index 01d3da19af53..65ce5d63456b 100644 --- a/src/components/Charts/LineChart/index.tsx +++ b/src/components/Charts/LineChart/index.tsx @@ -1,21 +1,27 @@ -import ChartWidthBox from '@components/Charts/components/ChartWidthBox'; +import ChartFrame from '@components/Charts/components/ChartFrame'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; +import {getCartesianChartHeight} from '@components/Charts/utils/chartHeights'; import React from 'react'; import type {LineChartProps} from './LineChartContent'; const getLineChartContent = () => import('./LineChartContent'); + function LineChart(props: LineChartProps) { return ( - + 0} + getLoadingHeight={() => getCartesianChartHeight()} + > {(chartWidth) => ( )} - + ); } diff --git a/src/components/Charts/PieChart/PieChartContent.tsx b/src/components/Charts/PieChart/PieChartContent.tsx index e1ba58eaf205..e2499c27bb9b 100644 --- a/src/components/Charts/PieChart/PieChartContent.tsx +++ b/src/components/Charts/PieChart/PieChartContent.tsx @@ -1,8 +1,9 @@ -import ActivityIndicator from '@components/ActivityIndicator'; +import {useReportChartLoading} from '@components/Charts/components/ChartReveal'; import ChartTooltipLayer from '@components/Charts/components/ChartTooltipLayer'; import {CHART_GESTURE_TOUCH_ACTION, 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 {hasRoomForSideLegend} from '@components/Charts/utils/chartHeights'; import VictoryTheme, {CHART_CONTENT_MIN_HEIGHT} from '@components/Charts/VictoryTheme'; import Text from '@components/Text'; @@ -65,7 +66,7 @@ function PieChartCanvas({data, series, valueUnit, valueUnitPosition, onSlicePres const canvasHeight = CHART_CONTENT_MIN_HEIGHT; // Side by side when the legend keeps at least its minimum width next to a square donut. Otherwise stacked. - const isLegendSideBySide = shouldUseResponsiveLegend && shouldShowLegend && chartWidth - canvasHeight - VictoryTheme.pie.legendGap >= VictoryTheme.pie.legendMinWidth; + const isLegendSideBySide = shouldUseResponsiveLegend && shouldShowLegend && hasRoomForSideLegend(chartWidth); const donutWidth = isLegendSideBySide ? canvasHeight : chartWidth; const radius = Math.min(donutWidth, canvasHeight) / 2; @@ -206,6 +207,7 @@ function PieChartCanvas({data, series, valueUnit, valueUnitPosition, onSlicePres // One legend entry: a color dot plus the slice name, wired to drive the tooltip on hover. // The responsive variant gives each slice its own row with the dot aligned to the first line. The default variant is an inline item for the wrapping row below the donut. + // `getPieChartLoadingHeight` reserves the wrapping row as one line of normal-size text, so the label has to keep that size. const renderLegendEntry = (slice: PieSlice, isResponsiveRow: boolean) => { const dot = ; return ( @@ -284,6 +286,7 @@ function PieChartCanvas({data, series, valueUnit, valueUnitPosition, onSlicePres ); + // `getPieChartLoadingHeight` rebuilds the height of both layouts below from the same theme values, so update it with any change to their structure or spacing. if (shouldUseResponsiveLegend) { return ( @@ -304,18 +307,10 @@ function PieChartCanvas({data, series, valueUnit, valueUnitPosition, onSlicePres } function PieChartContent({isLoading = false, chartWidth, ...canvasProps}: PieChartContentProps) { - const styles = useThemeStyles(); - - if (!isLoading && canvasProps.data.length === 0) { - return null; - } + useReportChartLoading(isLoading); if (isLoading || chartWidth === null) { - return ( - - - - ); + return null; } return ( diff --git a/src/components/Charts/PieChart/index.native.tsx b/src/components/Charts/PieChart/index.native.tsx index 97d224a27cbf..2379b4230de5 100644 --- a/src/components/Charts/PieChart/index.native.tsx +++ b/src/components/Charts/PieChart/index.native.tsx @@ -1,4 +1,7 @@ -import ChartWidthBox from '@components/Charts/components/ChartWidthBox'; +import ChartFrame from '@components/Charts/components/ChartFrame'; +import {getPieChartLoadingHeight} from '@components/Charts/utils/chartHeights'; + +import useResponsiveLayout from '@hooks/useResponsiveLayout'; import React from 'react'; @@ -7,15 +10,21 @@ import type {PieChartProps} from './PieChartContent'; import PieChartContent from './PieChartContent'; function PieChart(props: PieChartProps) { + const {shouldUseNarrowLayout} = useResponsiveLayout(); + return ( - + 0} + getLoadingHeight={(chartWidth) => getPieChartLoadingHeight(chartWidth, shouldUseNarrowLayout, props.shouldShowLegend, props.shouldUseResponsiveLegend)} + > {(chartWidth) => ( )} - + ); } diff --git a/src/components/Charts/PieChart/index.tsx b/src/components/Charts/PieChart/index.tsx index 1441e2cdc847..2ede2e630640 100644 --- a/src/components/Charts/PieChart/index.tsx +++ b/src/components/Charts/PieChart/index.tsx @@ -1,5 +1,8 @@ -import ChartWidthBox from '@components/Charts/components/ChartWidthBox'; +import ChartFrame from '@components/Charts/components/ChartFrame'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; +import {getPieChartLoadingHeight} from '@components/Charts/utils/chartHeights'; + +import useResponsiveLayout from '@hooks/useResponsiveLayout'; import React from 'react'; @@ -8,15 +11,21 @@ import type {PieChartProps} from './PieChartContent'; const getPieChartContent = () => import('./PieChartContent'); function PieChart(props: PieChartProps) { + const {shouldUseNarrowLayout} = useResponsiveLayout(); + return ( - + 0} + getLoadingHeight={(chartWidth) => getPieChartLoadingHeight(chartWidth, shouldUseNarrowLayout, props.shouldShowLegend, props.shouldUseResponsiveLegend)} + > {(chartWidth) => ( )} - + ); } diff --git a/src/components/Charts/SkiaWebChart/index.tsx b/src/components/Charts/SkiaWebChart/index.tsx index 8268fa6de3c9..e107f241aca1 100644 --- a/src/components/Charts/SkiaWebChart/index.tsx +++ b/src/components/Charts/SkiaWebChart/index.tsx @@ -1,4 +1,5 @@ import ActivityIndicator from '@components/ActivityIndicator'; +import {useIsInsideChartReveal, useReportChartLoading} from '@components/Charts/components/ChartReveal'; import Icon from '@components/Icon'; import Text from '@components/Text'; @@ -30,8 +31,6 @@ type SkiaWebChartProps = { /** Props forwarded to the lazily-loaded chart component. */ componentProps: TProps; - - /** Identifies the loading skeleton span for telemetry. */ }; type ChartUnavailableProps = { @@ -51,6 +50,9 @@ function ChartUnavailable({description = 'common.webGLNotSupported'}: ChartUnava const {translate} = useLocalize(); const illustrations = useMemoizedLazyIllustrations(['MonitorSyncNo']); + // This state never loads, and Skia draws none of it, so an enclosing ChartReveal shows it at once + useReportChartLoading(false, false); + return ( ; } @@ -97,9 +108,9 @@ const logSkiaLoadError = (error: Error, info: {componentStack?: string | null}) function SkiaWebChart({getComponent, componentProps}: SkiaWebChartProps) { const styles = useThemeStyles(); const containerRef = useRef(null); + const isInsideChartReveal = useIsInsideChartReveal(); - // Probe once per mount (not per render) so re-rendering doesn't repeatedly create WebGL contexts, - // while a fresh chart still re-checks capability instead of trusting a stale session-wide result. + // Probed on each mount, so a fresh chart 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 @@ -111,7 +122,9 @@ function SkiaWebChart({getComponent, componentProps}: Ski return ; } - const fallback = ( + const fallback = isInsideChartReveal ? ( + + ) : ( diff --git a/src/components/Charts/VictoryTheme.ts b/src/components/Charts/VictoryTheme.ts index 6f437df40d64..d39144751941 100644 --- a/src/components/Charts/VictoryTheme.ts +++ b/src/components/Charts/VictoryTheme.ts @@ -3,6 +3,7 @@ */ import type {ColorScheme} from '@styles/index'; import colors from '@styles/theme/colors'; +import variables from '@styles/variables'; import CONST from '@src/CONST'; @@ -111,6 +112,8 @@ const VictoryTheme = { legendGap: 24, /** Gap (px) between the donut and the legend below it in the stacked layout */ legendStackGap: 32, + /** Gap (px) between the rows of the side and stacked legends */ + legendRowGap: 12, }, } as const; @@ -123,6 +126,13 @@ const CARTESIAN_Y_AXIS = { /** Minimum height for the chart content area (bars, Y-axis, grid lines) */ const CHART_CONTENT_MIN_HEIGHT = 250; +/** 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; + +const X_AXIS_LABEL_LINE_HEIGHT = variables.iconSizeExtraSmall * LABEL_LINE_HEIGHT_RATIO; + +const X_AXIS_LABEL_MIN_HEIGHT = Math.ceil(X_AXIS_LABEL_LINE_HEIGHT); + /** 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; @@ -173,6 +183,8 @@ export { CHART_CONTENT_MIN_HEIGHT, X_AXIS_LABEL_MIN_GAP, CHART_Y_SCALE_HEIGHT, + X_AXIS_LABEL_LINE_HEIGHT, + X_AXIS_LABEL_MIN_HEIGHT, LABEL_ROTATIONS, SIN_45, LABEL_PADDING, diff --git a/src/components/Charts/components/ChartFrame.tsx b/src/components/Charts/components/ChartFrame.tsx new file mode 100644 index 000000000000..c58f7838fdd0 --- /dev/null +++ b/src/components/Charts/components/ChartFrame.tsx @@ -0,0 +1,42 @@ +/** + * Every chart entry point renders its chart inside this frame, so bar, line and pie charts measure their card and reveal the chart the same way on web and native. + */ +import type {ReactNode} from 'react'; + +import React from 'react'; + +import ChartReveal from './ChartReveal'; +import ChartWidthBox from './ChartWidthBox'; + +type ChartFrameProps = { + isLoading?: boolean; + hasData: boolean; + + /** Must match the loaded chart's height, or the card jumps at the reveal. Receives the card's width, or null until the card has been measured. */ + getLoadingHeight: (chartWidth: number | null) => number; + + /** Receives the card's width, or null until the card has been measured. */ + children: (chartWidth: number | null) => ReactNode; +}; + +function ChartFrame({isLoading = false, hasData, getLoadingHeight, children}: ChartFrameProps) { + // A chart that loads empty collapses at once, so it never starts the engine download or holds a spinner over nothing + if (!isLoading && !hasData) { + return null; + } + + return ( + + {(chartWidth) => ( + + {children(chartWidth)} + + )} + + ); +} + +export default ChartFrame; diff --git a/src/components/Charts/components/ChartReveal.tsx b/src/components/Charts/components/ChartReveal.tsx new file mode 100644 index 000000000000..a12074237c26 --- /dev/null +++ b/src/components/Charts/components/ChartReveal.tsx @@ -0,0 +1,139 @@ +import ActivityIndicator from '@components/ActivityIndicator'; + +import useStyleUtils from '@hooks/useStyleUtils'; +import useThemeStyles from '@hooks/useThemeStyles'; + +import CONST from '@src/CONST'; + +import type {ReactNode} from 'react'; + +import React, {createContext, useContext, useEffect, useLayoutEffect, useState} from 'react'; +import {View} from 'react-native'; +import Animated, {useAnimatedStyle, useSharedValue, withTiming} from 'react-native-reanimated'; +import {scheduleOnRN, scheduleOnUI} from 'react-native-worklets'; + +/** + * Skia draws a chart a few frames after it mounts: 2 to 4 on web. + * Counted in frames because a stalled main thread delays the draw by frames, which a timer would not wait for. + * Counted on the UI thread, where native Skia draws, because frames counted on a busy JS thread run seconds past the draw. + */ +const HOLD_FRAMES = 5; + +type ChartLoadState = { + isLoading: boolean; + isDrawnBySkia: boolean; +}; + +const ChartLoadingContext = createContext<((loadState: ChartLoadState) => void) | null>(null); + +function useReportChartLoading(isLoading: boolean, isDrawnBySkia = true) { + const setLoadState = useContext(ChartLoadingContext); + + // Before paint, so no frame shows the chart and the in-flow spinner stacked, or neither of them + useLayoutEffect(() => { + setLoadState?.({isLoading, isDrawnBySkia}); + }, [setLoadState, isLoading, isDrawnBySkia]); +} + +function useIsInsideChartReveal() { + return useContext(ChartLoadingContext) !== null; +} + +type ChartRevealProps = { + /** Must match the loaded chart's height, or the card jumps at the reveal. */ + loadingHeight: number; + + /** Until the card is measured there is no canvas for Skia to draw, so the chart is not ready to reveal. */ + isCardMeasured: boolean; + + /** Must report through `useReportChartLoading`, or the spinner never goes away. */ + children: ReactNode; +}; + +function ChartReveal({loadingHeight, isCardMeasured, children}: ChartRevealProps) { + const styles = useThemeStyles(); + const StyleUtils = useStyleUtils(); + const [loadState, setLoadState] = useState(null); + const [hasReportedLoading, setHasReportedLoading] = useState(false); + const [isRevealed, setIsRevealed] = useState(false); + const opacity = useSharedValue(0); + const fadeStyle = useAnimatedStyle(() => ({opacity: opacity.get()})); + + // Hidden on the UI thread as the fade starts, because unmounting it waits for a JS thread that can be busy for seconds + const spinnerStyle = useAnimatedStyle(() => ({opacity: opacity.get() > 0 ? 0 : 1})); + + // Written on the UI thread, which counts the hold, so that the cleanup there can cancel the hold's next frame + const holdFrameID = useSharedValue(0); + + const isReady = !!loadState && !loadState.isLoading && isCardMeasured; + + // A chart that mounts with everything in memory waits only for its card's first layout to report the width, + // so it shows no spinner and fades in at once. Only a chart that has shown the spinner holds it until Skia has drawn. + const isSpinnerShown = !loadState || hasReportedLoading; + const shouldHold = hasReportedLoading && !!loadState?.isDrawnBySkia; + + // Adjusted during render, not in the effect below, so the spinner changes in the same commit as what the chart reports + if (loadState?.isLoading && !hasReportedLoading) { + setHasReportedLoading(true); + } + if (!isReady && isRevealed) { + setIsRevealed(false); + } + if (isReady && !shouldHold && !isRevealed) { + setIsRevealed(true); + } + + useEffect(() => { + if (!isReady) { + opacity.set(0); + return; + } + + // Read here so that the worklet below captures a number rather than all of CONST + const duration = CONST.ANIMATED_TRANSITION; + if (!shouldHold) { + opacity.set(withTiming(1, {duration})); + return; + } + + scheduleOnUI(() => { + 'worklet'; + + let framesLeft = HOLD_FRAMES; + const countFrame = () => { + framesLeft -= 1; + if (framesLeft > 0) { + holdFrameID.set(requestAnimationFrame(countFrame)); + return; + } + opacity.set(withTiming(1, {duration})); + scheduleOnRN(setIsRevealed, true); + }; + holdFrameID.set(requestAnimationFrame(countFrame)); + }); + + return () => { + scheduleOnUI(() => { + 'worklet'; + + cancelAnimationFrame(holdFrameID.get()); + }); + }; + }, [isReady, shouldHold, opacity, holdFrameID]); + + return ( + + + {children} + {!isRevealed && ( + + {isSpinnerShown && } + + )} + + + ); +} + +export default ChartReveal; +export {HOLD_FRAMES, useIsInsideChartReveal, useReportChartLoading}; diff --git a/src/components/Charts/hooks/useReadyCartesianCanvas.ts b/src/components/Charts/hooks/useReadyCartesianCanvas.ts index 13e7d3288bfa..ab8ab352b666 100644 --- a/src/components/Charts/hooks/useReadyCartesianCanvas.ts +++ b/src/components/Charts/hooks/useReadyCartesianCanvas.ts @@ -1,3 +1,4 @@ +import {useReportChartLoading} from '@components/Charts/components/ChartReveal'; import {useChartFontManager} from '@components/Charts/context/ChartFontsContext'; import type {SkTypefaceFontProvider} from '@shopify/react-native-skia'; @@ -10,7 +11,10 @@ type CartesianCanvasInputs = { function useReadyCartesianCanvas(isLoading: boolean, chartWidth: number | null): CartesianCanvasInputs | null { const fontManager = useChartFontManager(); - if (isLoading || !fontManager || chartWidth === null) { + const isChartLoading = isLoading || !fontManager; + useReportChartLoading(isChartLoading); + + if (isChartLoading || chartWidth === null) { return null; } diff --git a/src/components/Charts/utils/chartHeights.ts b/src/components/Charts/utils/chartHeights.ts new file mode 100644 index 000000000000..df1d6e5fb964 --- /dev/null +++ b/src/components/Charts/utils/chartHeights.ts @@ -0,0 +1,52 @@ +import VictoryTheme, {CHART_CONTENT_MIN_HEIGHT, MAX_BAR_LABEL_LINES, X_AXIS_LABEL_LINE_HEIGHT, X_AXIS_LABEL_MIN_HEIGHT} from '@components/Charts/VictoryTheme'; + +import variables from '@styles/variables'; + +import CONST from '@src/CONST'; + +// Imported before the chart engine has loaded, so this file must not import Skia. + +function getXAxisLabelSpace(xAxisLabelHeight = 0): number { + return VictoryTheme.axis.xAxisLabelGap + Math.max(xAxisLabelHeight, X_AXIS_LABEL_MIN_HEIGHT); +} + +function getCartesianChartHeight(xAxisLabelHeight = 0): number { + return CHART_CONTENT_MIN_HEIGHT + getXAxisLabelSpace(xAxisLabelHeight); +} + +function getBarChartHeight(shouldShowLabels = true, xAxisLabelHeight = 0): number { + return shouldShowLabels ? getCartesianChartHeight(xAxisLabelHeight) : CHART_CONTENT_MIN_HEIGHT; +} + +// Wrapped labels keep every line of label space even when the names fit on fewer, so their height is known before the chart measures them. +// Mirrors the wrap height `VerticalBarChartContent` draws, so update the two together. +function getBarChartLoadingHeight(shouldShowLabels = true, shouldWrapLabels = false): number { + return getBarChartHeight(shouldShowLabels, shouldWrapLabels ? MAX_BAR_LABEL_LINES * X_AXIS_LABEL_LINE_HEIGHT : 0); +} + +/** Whether the card leaves the responsive legend its minimum width beside a square donut. */ +function hasRoomForSideLegend(chartWidth: number): boolean { + return chartWidth - CHART_CONTENT_MIN_HEIGHT - VictoryTheme.pie.legendGap >= VictoryTheme.pie.legendMinWidth; +} + +// Mirrors the legend layouts `PieChartContent` draws with the `pieChartLegend*` and `pieChartResponsive*` styles, so update it with any change to their structure or spacing. +function getPieChartLoadingHeight(chartWidth: number | null, shouldUseNarrowLayout: boolean, shouldShowLegend = true, shouldUseResponsiveLegend = false): number { + if (shouldShowLegend && shouldUseResponsiveLegend) { + // Until the card reports its width, the layout stands in for it: phone cards are too narrow for a side legend, and typical desktop cards fit one + const isLegendSideBySide = chartWidth === null ? !shouldUseNarrowLayout : hasRoomForSideLegend(chartWidth); + if (isLegendSideBySide) { + return CHART_CONTENT_MIN_HEIGHT; + } + + // The slice count arrives with the data, so this reserves a full Home top-N list, which most cards fill. + // A row is as tall as the taller of its dot column and its label's first line. + const rowCount = CONST.SEARCH.TOP_SEARCH_LIMIT; + const rowHeight = Math.max(variables.lineHeightXLarge, variables.fontSizeNormalHeight); + return CHART_CONTENT_MIN_HEIGHT + VictoryTheme.pie.legendStackGap + rowCount * rowHeight + (rowCount - 1) * VictoryTheme.pie.legendRowGap; + } + + const legendFirstRowHeight = shouldShowLegend ? variables.pieChartLegendMargin + variables.fontSizeNormalHeight : 0; + return CHART_CONTENT_MIN_HEIGHT + legendFirstRowHeight; +} + +export {getXAxisLabelSpace, getCartesianChartHeight, getBarChartHeight, getBarChartLoadingHeight, hasRoomForSideLegend, getPieChartLoadingHeight}; diff --git a/src/pages/home/HomePageSkeleton.tsx b/src/pages/home/HomePageSkeleton.tsx index 9aeeaf39f70e..48d70e0289ce 100644 --- a/src/pages/home/HomePageSkeleton.tsx +++ b/src/pages/home/HomePageSkeleton.tsx @@ -1,5 +1,5 @@ import ActivityIndicator from '@components/ActivityIndicator'; -import {CHART_CONTENT_MIN_HEIGHT} from '@components/Charts/VictoryTheme'; +import {getCartesianChartHeight} from '@components/Charts/utils/chartHeights'; import SkeletonRect from '@components/SkeletonRect'; import ItemListSkeletonView from '@components/Skeletons/ItemListSkeletonView'; import SkeletonTextLine, {BAR_HEIGHT} from '@components/Skeletons/SkeletonTextLine'; @@ -19,6 +19,7 @@ import React from 'react'; import {View} from 'react-native'; import {ICON_SLOT_SIZE, useWidgetSkeletonRowGeometry, WidgetSkeletonRowIcon} from './common/widgetSkeletonRow'; +import useInsightChartPaddingStyle from './InsightsSection/useInsightChartPaddingStyle'; // The stacked pair of text lines the two-bar rows stand in for: a merchant line over a muted label line. const FIRST_LINE_HEIGHT = lineHeightScale.text; @@ -29,8 +30,8 @@ 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. -const SPINNER_CARD_HEIGHT = CHART_CONTENT_MIN_HEIGHT; +// Home's first insight, spend over time, is a line chart, which holds its loading spinner at this height. +const SPINNER_CARD_HEIGHT = getCartesianChartHeight(); const ROWS_PER_LIST_CARD = 3; const ROWS_PER_TABLE_CARD = CONST.HOME.SECTION_VISIBLE_LIMIT; @@ -108,9 +109,10 @@ function renderIconTwoBarWithTrailingRow(args: TrailingSkeletonRowArgs) { type HomePageSkeletonCardProps = { children: React.ReactNode; + hasBottomPadding?: boolean; }; -function HomePageSkeletonCard({children}: HomePageSkeletonCardProps) { +function HomePageSkeletonCard({children, hasBottomPadding = true}: HomePageSkeletonCardProps) { const styles = useThemeStyles(); const {shouldUseNarrowLayout} = useResponsiveLayout(); @@ -123,7 +125,7 @@ function HomePageSkeletonCard({children}: HomePageSkeletonCardProps) { barWidth={CARD_TITLE_WIDTH} /> } - containerStyles={styles.getWidgetContainerBottomPaddingStyle(shouldUseNarrowLayout)} + containerStyles={hasBottomPadding && styles.getWidgetContainerBottomPaddingStyle(shouldUseNarrowLayout)} > {children} @@ -176,14 +178,17 @@ function HomePageSkeletonTableCard() { function HomePageSkeletonSpinnerCard() { const styles = useThemeStyles(); const StyleUtils = useStyleUtils(); + const chartPaddingStyle = useInsightChartPaddingStyle(); return ( - - - + + + + + ); diff --git a/src/pages/home/InsightsSection/InsightsSectionContent.tsx b/src/pages/home/InsightsSection/InsightsSectionContent.tsx index a6a15e9a675d..ac86a03d327c 100644 --- a/src/pages/home/InsightsSection/InsightsSectionContent.tsx +++ b/src/pages/home/InsightsSection/InsightsSectionContent.tsx @@ -6,13 +6,10 @@ import SearchChartView from '@components/Search/SearchChartView'; import WidgetContainer from '@components/WidgetContainer'; import WidgetHeaderMenu from '@components/WidgetHeaderMenu'; -import useLayoutSpacing from '@hooks/useLayoutSpacing'; import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset'; import useLocalize from '@hooks/useLocalize'; import useOnyx from '@hooks/useOnyx'; import usePermissions from '@hooks/usePermissions'; -import useResponsiveLayout from '@hooks/useResponsiveLayout'; -import useThemeStyles from '@hooks/useThemeStyles'; import {setNameValuePair} from '@libs/actions/User'; import {canUseTouchScreen} from '@libs/DeviceCapabilities'; @@ -29,14 +26,13 @@ import {View} from 'react-native'; import InsightTitleDropdown from './InsightTitleDropdown'; import useHomeInsightConfigs from './useHomeInsightConfigs'; +import useInsightChartPaddingStyle from './useInsightChartPaddingStyle'; import useInsightData from './useInsightData'; function InsightsSectionContent() { - const styles = useThemeStyles(); - const {cardPaddingHorizontal, cardPaddingBottom} = useLayoutSpacing(); + const chartPaddingStyle = useInsightChartPaddingStyle(); const {translate} = useLocalize(); const icons = useMemoizedLazyExpensifyIcons(['Expand']); - const {shouldUseNarrowLayout} = useResponsiveLayout(); const {isBetaEnabled} = usePermissions(); const isInsightsPageEnabled = isBetaEnabled(CONST.BETAS.INSIGHTS_PAGE); @@ -93,7 +89,7 @@ function InsightsSectionContent() { {state === INSIGHTS_CHART_STATE.EMPTY && } {state === INSIGHTS_CHART_STATE.ERROR && } {(state === INSIGHTS_CHART_STATE.LOADING || state === INSIGHTS_CHART_STATE.READY) && ( - + { + const styles = useThemeStyles(); + const {cardPaddingHorizontal, cardPaddingBottom} = useLayoutSpacing(); + const {shouldUseNarrowLayout} = useResponsiveLayout(); + + return [cardPaddingHorizontal, cardPaddingBottom, !shouldUseNarrowLayout && styles.pt3]; +} + +export default useInsightChartPaddingStyle; diff --git a/src/styles/index.ts b/src/styles/index.ts index e8514c66f598..2ca45f1fe174 100644 --- a/src/styles/index.ts +++ b/src/styles/index.ts @@ -7082,8 +7082,8 @@ const staticStyles = (theme: ThemeColors) => display: 'flex', justifyContent: 'center', alignItems: 'center', - marginHorizontal: variables.qrShareHorizontalPadding, - marginTop: variables.qrShareHorizontalPadding, + marginHorizontal: variables.pieChartLegendMargin, + marginTop: variables.pieChartLegendMargin, flexDirection: 'row', flexWrap: 'wrap', rowGap: 8, @@ -7104,6 +7104,7 @@ const staticStyles = (theme: ThemeColors) => width: 12, height: 12, }, + // `getPieChartLoadingHeight` sizes the loading chart from the responsive legend styles below, so update it with them. pieChartResponsiveRow: { flexDirection: 'row', alignItems: 'center', @@ -7117,12 +7118,12 @@ const staticStyles = (theme: ThemeColors) => pieChartLegendSide: { width: VictoryTheme.pie.legendMinWidth, justifyContent: 'center', - rowGap: 12, + rowGap: VictoryTheme.pie.legendRowGap, }, pieChartLegendBelow: { alignSelf: 'stretch', marginTop: VictoryTheme.pie.legendStackGap, - rowGap: 12, + rowGap: VictoryTheme.pie.legendRowGap, }, pieChartLegendRow: { flexDirection: 'row', diff --git a/src/styles/variables.ts b/src/styles/variables.ts index c93926165e36..a85daecbbb9d 100644 --- a/src/styles/variables.ts +++ b/src/styles/variables.ts @@ -625,4 +625,5 @@ export default { editableCellEditButtonWidth: 28 + 4, // How short a value has to be before that edit button is worth reserving room for. narrowEditableContentWidth: 60, + pieChartLegendMargin: 32, } as const; diff --git a/tests/unit/components/Charts/SkiaWebChart.test.tsx b/tests/unit/components/Charts/SkiaWebChart.test.tsx index 01a592718d5c..ffbf1eda6066 100644 --- a/tests/unit/components/Charts/SkiaWebChart.test.tsx +++ b/tests/unit/components/Charts/SkiaWebChart.test.tsx @@ -1,11 +1,15 @@ import {render, screen} from '@testing-library/react-native'; +import ActivityIndicator from '@components/ActivityIndicator'; +import ChartReveal, {useReportChartLoading} from '@components/Charts/components/ChartReveal'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; import isSkiaWebSupported from '@components/Charts/SkiaWebChart/isSkiaWebSupported'; +import {getCartesianChartHeight} from '@components/Charts/utils/chartHeights'; import * as Sentry from '@sentry/react-native'; 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()); @@ -27,13 +31,32 @@ jest.mock('@components/Icon', () => ({ default: () => null, })); +const CHART_LOADING_HEIGHT = getCartesianChartHeight(); + const mockIsSkiaWebSupported = jest.mocked(isSkiaWebSupported); const mockWithSkiaWeb = jest.mocked(WithSkiaWeb); const mockCaptureException = jest.mocked(Sentry.captureException); +function ReadyChart() { + useReportChartLoading(false); + return null; +} + describe('SkiaWebChart', () => { const getComponent = () => Promise.resolve({default: () => null}); + const renderRevealedChart = () => ( + + + + ); + afterEach(() => { jest.clearAllMocks(); }); @@ -53,6 +76,40 @@ describe('SkiaWebChart', () => { expect(mockWithSkiaWeb).not.toHaveBeenCalled(); }); + it('should show the failed-to-load message under a reveal without holding its spinner', () => { + // Given a chart under a reveal whose spinner is up while Skia loads + mockIsSkiaWebSupported.mockReturnValue(true); + mockWithSkiaWeb.mockImplementation(({fallback}) => {fallback}); + const {rerender} = render(renderRevealedChart()); + + // When CanvasKit fails to load. React logs caught render errors to console.error, so mock it to keep the test output clean. + mockWithSkiaWeb.mockImplementation(() => { + throw new Error('CanvasKit initialized without its bindings'); + }); + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + rerender(renderRevealedChart()); + + // Then the message replaces the spinner in the same render, because the hold waits for a Skia draw this state never makes + expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); + expect(screen.getByText('common.unableToDisplayChart')).toBeTruthy(); + + consoleErrorSpy.mockRestore(); + }); + + it('should hold the reveal spinner when the chart was still loading Skia at mount', () => { + // Given a browser with WebGL where Skia has not loaded yet, so the chart shows its fallback under a reveal + mockIsSkiaWebSupported.mockReturnValue(true); + mockWithSkiaWeb.mockImplementation(({fallback}) => {fallback}); + const {rerender} = render(renderRevealedChart()); + + // When Skia loads and the chart reports it is ready + mockWithSkiaWeb.mockImplementation(() => ); + rerender(renderRevealedChart()); + + // Then the spinner the fallback put up stays for the hold, because taking it down before Skia draws would show an empty card + expect(screen.UNSAFE_queryByType(ActivityIndicator)).not.toBeNull(); + }); + it('should mount Skia when WebGL is supported', () => { mockIsSkiaWebSupported.mockReturnValue(true); diff --git a/tests/unit/components/Charts/chartReveal.test.tsx b/tests/unit/components/Charts/chartReveal.test.tsx new file mode 100644 index 000000000000..426a38ec2fb4 --- /dev/null +++ b/tests/unit/components/Charts/chartReveal.test.tsx @@ -0,0 +1,208 @@ +import {act, render, screen} from '@testing-library/react-native'; + +import ActivityIndicator from '@components/ActivityIndicator'; +import ChartReveal, {HOLD_FRAMES, useReportChartLoading} from '@components/Charts/components/ChartReveal'; +import {getCartesianChartHeight} from '@components/Charts/utils/chartHeights'; +import Text from '@components/Text'; + +import React from 'react'; + +const LOADING_HEIGHT = getCartesianChartHeight(); + +let pendingFrames = new Map(); +let lastFrameID = 0; + +/** The worklets mock runs UI-thread work on a timer and calls back into React in a microtask, which the fake timers below hold */ +function flushWorklets() { + act(() => { + jest.runOnlyPendingTimers(); + }); +} + +function runFrames(count: number) { + flushWorklets(); + for (let frame = 0; frame < count; frame++) { + const callbacks = [...pendingFrames.values()]; + pendingFrames.clear(); + act(() => { + for (const callback of callbacks) { + callback(0); + } + }); + } + flushWorklets(); +} + +type ChartBodyProps = { + isLoading: boolean; + label: string; +}; + +function ChartBody({isLoading, label}: ChartBodyProps) { + useReportChartLoading(isLoading); + return isLoading ? null : {label}; +} + +function EngineDownloading() { + return null; +} + +function renderChart(isLoading: boolean, label = 'chart', isCardMeasured = true) { + return ( + + + + ); +} + +describe('ChartReveal', () => { + beforeEach(() => { + jest.useFakeTimers(); + pendingFrames = new Map(); + jest.spyOn(global, 'requestAnimationFrame').mockImplementation((callback) => { + lastFrameID += 1; + pendingFrames.set(lastFrameID, callback); + return lastFrameID; + }); + jest.spyOn(global, 'cancelAnimationFrame').mockImplementation((frameID) => { + if (frameID === null || frameID === undefined) { + return; + } + pendingFrames.delete(frameID); + }); + }); + + afterEach(() => { + jest.restoreAllMocks(); + jest.useRealTimers(); + }); + + it('should fade the chart in without a spinner when it is only waiting for its card to be measured', () => { + // Given a chart whose data and fonts are already in memory, mounted before its card has been measured + const {rerender} = render(renderChart(false, 'chart', false)); + + // Then nothing covers the card, because the width comes from the card's first layout and a spinner would only flash + expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); + + // When the card is measured + rerender(renderChart(false)); + + // Then the chart starts fading in at once rather than after a hold, since there is no spinner to keep up while Skia draws + expect(screen.getByText('chart')).toBeTruthy(); + expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); + expect(pendingFrames.size).toBe(0); + }); + + it('should keep the spinner over the mounted chart until the last held frame', () => { + // Given a chart that was loading and whose data has just become ready + const {rerender} = render(renderChart(true)); + rerender(renderChart(false)); + + // When every held frame but the last passes + runFrames(HOLD_FRAMES - 1); + + // Then the chart is mounted but still covered, because Skia may not have drawn it yet + expect(screen.getByText('chart')).toBeTruthy(); + expect(screen.UNSAFE_queryByType(ActivityIndicator)).not.toBeNull(); + + // When the last held frame passes + runFrames(1); + + // Then the spinner goes away, since the hold has outlasted Skia's first draw + expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); + }); + + it('should keep one spinner from the engine download until the reveal', () => { + // Given a chart whose engine is still downloading, so nothing below has reported yet + const {rerender} = render( + + + , + ); + const downloadSpinner = screen.UNSAFE_getByType(ActivityIndicator); + runFrames(HOLD_FRAMES); + + // When the chart replaces the download placeholder, loads, and then gets its data + rerender(renderChart(true)); + const loadingSpinner = screen.UNSAFE_getByType(ActivityIndicator); + rerender(renderChart(false)); + + // Then the spinner covering the hold is the one shown during the download, + // because a new one would restart its rotation at each of those steps + expect(loadingSpinner).toBe(downloadSpinner); + expect(screen.UNSAFE_getByType(ActivityIndicator)).toBe(downloadSpinner); + }); + + it('should not hold again when a revealed chart gets new data', () => { + // Given a chart that has been revealed after loading + const {rerender} = render(renderChart(true)); + rerender(renderChart(false)); + runFrames(HOLD_FRAMES); + + // When its data changes without it loading again + rerender(renderChart(false, 'updated chart')); + + // Then it updates in place, since Skia redraws a mounted chart without a blank frame + expect(screen.getByText('updated chart')).toBeTruthy(); + expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); + }); + + it('should hold when a chart that faded in without a spinner reloads', () => { + // Given a chart that faded in as soon as its card was measured + const {rerender} = render(renderChart(false, 'chart', false)); + rerender(renderChart(false)); + + // When it loads again and its new data arrives + rerender(renderChart(true)); + rerender(renderChart(false)); + + // Then the remounted chart is covered again, because Skia has to draw it from scratch + expect(screen.getByText('chart')).toBeTruthy(); + expect(screen.UNSAFE_queryByType(ActivityIndicator)).not.toBeNull(); + }); + + it('should restart the hold when a chart reloads before its hold ends', () => { + // Given a chart partway through its hold after loading + const {rerender} = render(renderChart(true)); + rerender(renderChart(false)); + runFrames(HOLD_FRAMES - 2); + + // When it loads again and its new data arrives + rerender(renderChart(true)); + rerender(renderChart(false)); + runFrames(HOLD_FRAMES - 1); + + // Then it is still covered one frame short of a full hold, because the frames counted before the reload + // belonged to a chart Skia no longer shows + expect(screen.UNSAFE_queryByType(ActivityIndicator)).not.toBeNull(); + + // When the last frame of the new hold passes + runFrames(1); + + // Then the spinner goes away, since the new hold has outlasted Skia's first draw of the remounted chart + expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); + }); + + it('should stop counting frames when the chart unmounts during its hold', () => { + // Given a chart partway through its hold after loading + const {rerender, unmount} = render(renderChart(true)); + rerender(renderChart(false)); + runFrames(1); + + // When it unmounts, as it does when the user leaves the page + unmount(); + flushWorklets(); + + // Then no frame is left waiting to reveal it, since that would update a component that is gone + expect(pendingFrames.size).toBe(0); + }); +}); diff --git a/tests/unit/components/Charts/chartWidthHandoff.test.tsx b/tests/unit/components/Charts/chartWidthHandoff.test.tsx index b34aefe53c3e..101084211368 100644 --- a/tests/unit/components/Charts/chartWidthHandoff.test.tsx +++ b/tests/unit/components/Charts/chartWidthHandoff.test.tsx @@ -59,6 +59,33 @@ describe('chart width handoff', () => { expect(getMeasuredBox()).toBeTruthy(); }); + it.each([ + ['bar', BarChart], + ['line', LineChart], + ['pie', PieChart], + ])('should collapse the %s chart as soon as it loads with no data', (_name, Chart) => { + // Given a loading chart + const {rerender} = render( + , + ); + + // When it finishes loading with nothing to draw + rerender( + , + ); + + // Then the spinner goes in the same render, because holding it for Skia's first draw would leave it over an empty box + expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); + }); + 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( @@ -83,7 +110,7 @@ describe('chart width handoff', () => { 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', () => { + it('should wait for the pie chart width without a spinner when its data is already in memory', () => { // Given a pie chart whose box has not reported a layout yet // When it renders with its data render( @@ -94,28 +121,37 @@ describe('chart width handoff', () => { />, ); - // Then the spinner stays and the chart does not mount, because it can only be sized once the width is known + // Then the chart waits for the width instead of measuring itself, because the reveal starts the fade in the render + // the card is measured in, and a chart that measures itself draws later than that. Nothing covers the card meanwhile, + // since the width comes from the card's first layout and a spinner would only flash. expect(PolarChart).not.toHaveBeenCalled(); - expect(screen.UNSAFE_queryByType(ActivityIndicator)).not.toBeNull(); + expect(screen.UNSAFE_queryByType(ActivityIndicator)).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( + // Given a pie chart that has mounted at the width measured while it was loading + const {rerender} = render( , ); const measuredBox = getMeasuredBox(); fireEvent(measuredBox, 'layout', {nativeEvent: {layout: {width: CONTAINER_WIDTH, height: CHART_CONTENT_MIN_HEIGHT}}}); + rerender( + , + ); // 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(screen.UNSAFE_queryByType(PolarChart)).not.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 a7e0ff3c83d5..9d0f6635cbb9 100644 --- a/tests/unit/components/Charts/utils.test.ts +++ b/tests/unit/components/Charts/utils.test.ts @@ -23,9 +23,22 @@ import { rotatedLabelYOffset, truncateLabel, } from '@components/Charts/utils'; -import VictoryTheme, {CHART_Y_SCALE_HEIGHT, DIAGONAL_ANGLE_RADIAN_THRESHOLD, LABEL_ROTATIONS, SIN_45} from '@components/Charts/VictoryTheme'; +import {getBarChartHeight, getBarChartLoadingHeight, getCartesianChartHeight, getPieChartLoadingHeight} from '@components/Charts/utils/chartHeights'; +import VictoryTheme, { + CHART_CONTENT_MIN_HEIGHT, + CHART_Y_SCALE_HEIGHT, + DIAGONAL_ANGLE_RADIAN_THRESHOLD, + LABEL_ROTATIONS, + MAX_BAR_LABEL_LINES, + SIN_45, + X_AXIS_LABEL_MIN_HEIGHT, +} from '@components/Charts/VictoryTheme'; import {CHART_SERIES_KEY} from '@components/Search/buildChartSeries'; +import variables from '@styles/variables'; + +import CONST from '@src/CONST'; + const LINE_HEIGHT = 16; describe('truncateLabel', () => { @@ -755,6 +768,99 @@ describe('getBarLayout', () => { }); }); +describe('getCartesianChartHeight', () => { + const ROTATED_LABEL_HEIGHT = 120; + + it('should give the loading box and a chart with one line of labels the same height', () => { + // Given the loading box, which has no labels to measure, and a loaded chart whose labels fit on one line + const loadingHeight = getCartesianChartHeight(); + + // When the loaded chart sizes its box from the measured label strip + const loadedHeight = getCartesianChartHeight(X_AXIS_LABEL_MIN_HEIGHT - 1); + + // Then both heights match, so the card does not jump when the chart replaces the spinner + expect(loadedHeight).toBe(loadingHeight); + }); + + it('should grow the box when rotated labels need more room than one line', () => { + // Given labels rotated so their strip is taller than one line of text + // When the chart sizes its box + const height = getCartesianChartHeight(ROTATED_LABEL_HEIGHT); + + // Then the box makes room for the whole strip, since clipping the labels would be worse than a jump + expect(height).toBe(CHART_CONTENT_MIN_HEIGHT + VictoryTheme.axis.xAxisLabelGap + ROTATED_LABEL_HEIGHT); + }); +}); + +describe('getBarChartLoadingHeight', () => { + // ExpensifyNeue's ascent plus descent at the 12px label size, (950 + 230) / 1000 of the em, which Skia measures for each label line + const SKIA_LABEL_LINE_HEIGHT = 14.16; + + it('should give the loading box the height of a chart whose labels wrap', () => { + // Given a bar chart with wrapped labels, which keeps two lines of label space whatever the names are + const loadedHeight = getBarChartHeight(true, MAX_BAR_LABEL_LINES * SKIA_LABEL_LINE_HEIGHT); + + // When the loading box is sized before the chart has measured anything + const loadingHeight = getBarChartLoadingHeight(true, true); + + // Then both heights match, so the card does not jump when the chart replaces the spinner + expect(loadingHeight).toBeCloseTo(loadedHeight, 5); + }); +}); + +describe('getPieChartLoadingHeight', () => { + // The narrowest card that fits the side legend: the 250px donut, the 24px gap and the 180px legend column + const SIDE_LEGEND_MIN_CARD_WIDTH = 454; + + it('should reserve the donut height for a side legend once the card is measured', () => { + // Given a responsive legend in a card just wide enough for it to sit beside the donut, on the narrow layout + // When the loading box is sized from the measured card width + const height = getPieChartLoadingHeight(SIDE_LEGEND_MIN_CARD_WIDTH, true, true, true); + + // Then it keeps the donut's height, since five legend rows fit beside it, and the measured width wins over the layout + expect(height).toBe(CHART_CONTENT_MIN_HEIGHT); + }); + + it('should reserve a full stacked legend under the donut when the card is too narrow for a side legend', () => { + // Given a responsive legend in a card 1px too narrow for the side legend, on the wide layout + // When the loading box is sized from the measured card width, before the slice count is known + const height = getPieChartLoadingHeight(SIDE_LEGEND_MIN_CARD_WIDTH - 1, false, true, true); + + // Then it reserves one row per slice Home can show, under the donut and the stack gap, since most cards fill the cap + const rowCount = CONST.SEARCH.TOP_SEARCH_LIMIT; + const legendHeight = rowCount * variables.fontSizeNormalHeight + (rowCount - 1) * VictoryTheme.pie.legendRowGap; + expect(height).toBe(CHART_CONTENT_MIN_HEIGHT + VictoryTheme.pie.legendStackGap + legendHeight); + }); + + it('should reserve only the donut when the legend is hidden', () => { + // Given a responsive pie with its legend turned off, in a card too narrow for a side legend + // When the loading box is sized + const height = getPieChartLoadingHeight(SIDE_LEGEND_MIN_CARD_WIDTH - 1, true, false, true); + + // Then it keeps the donut's height alone, since the chart draws no legend to stack + expect(height).toBe(CHART_CONTENT_MIN_HEIGHT); + }); + + it('should assume a stacked legend on the narrow layout until the card is measured', () => { + // Given a responsive legend whose card has not reported its width yet, on the narrow layout, where phone cards are too narrow for a side legend + // When the loading box is sized before the width lands + const height = getPieChartLoadingHeight(null, true, true, true); + + // Then it matches the stacked height a phone card measures into, so the box does not change when the width lands + const stackedHeight = getPieChartLoadingHeight(SIDE_LEGEND_MIN_CARD_WIDTH - 1, true, true, true); + expect(height).toBe(stackedHeight); + }); + + it('should assume a side legend on the wide layout until the card is measured', () => { + // Given a responsive legend whose card has not reported its width yet, on the wide layout, where a typical desktop card fits a side legend + // When the loading box is sized before the width lands + const height = getPieChartLoadingHeight(null, false, true, true); + + // Then it keeps the donut's height that a desktop card measures into, so the box does not change when the width lands + expect(height).toBe(CHART_CONTENT_MIN_HEIGHT); + }); +}); + describe('getCartesianPlotBounds', () => { const PADDING_LEFT = VictoryTheme.axis.padding.left;