Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: fixed

Reserve room for the first and last labels on a time axis, size the y-axis gutter from a pinned domain, and reserve nothing for a hidden y axis.
Original file line number Diff line number Diff line change
Expand Up @@ -123,18 +123,9 @@ describe( 'PieChart', () => {
test( 'hides labels when showLabels is false', () => {
renderWithTheme( { showLabels: false } );

// When showLabels is false, the chart should not display the data labels
// We filter out measurement elements by checking that text is not inside measurement element
const labelElements = screen.queryAllByText( ( content, element ) => {
// Check if this text element is not the measurement element
return (
( content === 'A' || content === 'B' ) &&
element?.id !== '__react_svg_text_measurement_id'
);
} );

// Labels should not be present in the rendered output (excluding measurement text)
expect( labelElements ).toHaveLength( 0 );
// A plain query, so this also fails if the measurement node stops being
// ignored — see tests/setup-text-measurement.js.
expect( screen.queryAllByText( /^[AB]$/ ) ).toHaveLength( 0 );
} );

test( 'shows labels when showLabels is explicitly true', () => {
Expand Down
176 changes: 173 additions & 3 deletions projects/js-packages/charts/src/hooks/test/use-chart-margin.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ jest.mock( '../../utils/get-longest-tick-width', () => ( {
getLongestTickWidth: ( ...args: unknown[] ) => mockGetLongestTickWidth( ...args ),
} ) );

// jsdom has no getComputedTextLength, so the real measurement always returns null.
const mockGetEdgeTickWidths = jest.fn();
jest.mock( '../../utils/get-edge-tick-widths', () => ( {
...jest.requireActual( '../../utils/get-edge-tick-widths' ),
getEdgeTickWidths: ( ...args: unknown[] ) => mockGetEdgeTickWidths( ...args ),
} ) );

describe( 'useChartMargin', () => {
const baseTheme = {
axisStyles: {
Expand Down Expand Up @@ -43,6 +50,8 @@ describe( 'useChartMargin', () => {
beforeEach( () => {
mockGetLongestTickWidth.mockReset();
mockGetLongestTickWidth.mockReturnValue( 40 );
mockGetEdgeTickWidths.mockReset();
mockGetEdgeTickWidths.mockReturnValue( { first: 0, last: 0 } );
} );

it( 'calculates left margin for left y axis', () => {
Expand All @@ -62,7 +71,7 @@ describe( 'useChartMargin', () => {
expect( mockGetLongestTickWidth ).toHaveBeenCalledWith(
expect.any( Array ),
options.axis.y.tickFormat,
theme.axisStyles.y.left.axisLabel
{ fontSize: '12px' }
);
// 40 label width + 8 tick length + ceil(11 * 0.25) label dx offset
expect( result.current.left ).toBe( 51 );
Expand All @@ -85,7 +94,7 @@ describe( 'useChartMargin', () => {
expect( mockGetLongestTickWidth ).toHaveBeenCalledWith(
expect.any( Array ),
options.axis.y.tickFormat,
theme.axisStyles.y.right.axisLabel
{ fontSize: '12px' }
);
// 40 label width + 8 tick length + ceil(11 * 0.25) label dx offset
expect( result.current.right ).toBe( 51 );
Expand All @@ -108,7 +117,7 @@ describe( 'useChartMargin', () => {
expect( mockGetLongestTickWidth ).toHaveBeenCalledWith(
[ 0, 1000 ],
options.axis.y.tickFormat,
theme.axisStyles.y.left.axisLabel
{ fontSize: '12px' }
);
} );

Expand Down Expand Up @@ -185,6 +194,167 @@ describe( 'useChartMargin', () => {
expect( result.current.bottom ).toBe( 25 );
} );

describe( 'x-axis edge tick labels', () => {
const tickFormat = ( value: number ) => new Date( value ).toDateString();
const tickValues = [ 1, 2, 3 ];
const datedXOptions = ( xOverrides = {} ) => ( {
...optionsBase,
axis: { ...optionsBase.axis, x: { tickValues, tickFormat, ...xOverrides } },
} );

it( 'reserves half of the last label on the right', () => {
mockGetEdgeTickWidths.mockReturnValue( { first: 0, last: 60 } );

const { result } = renderHook( () =>
useChartMargin( 300, datedXOptions(), data, baseTheme )
);

expect( result.current.right ).toBe( 30 );
} );

it( 'keeps the default right margin when the last label fits inside it', () => {
mockGetEdgeTickWidths.mockReturnValue( { first: 0, last: 30 } );

const { result } = renderHook( () =>
useChartMargin( 300, datedXOptions(), data, baseTheme )
);

expect( result.current.right ).toBe( 20 );
} );

it( 'widens the left margin past the y-axis reservation when the first label needs it', () => {
mockGetEdgeTickWidths.mockReturnValue( { first: 120, last: 0 } );

const { result } = renderHook( () =>
useChartMargin( 300, datedXOptions(), data, baseTheme )
);

// 60 for the label's overhanging half, over the 51 the y-axis ticks need.
expect( result.current.left ).toBe( 60 );
} );

it( 'measures the axis tick values with the x tick label style', () => {
const theme = {
...baseTheme,
axisStyles: {
...baseTheme.axisStyles,
x: {
bottom: { tickLabel: { fontSize: 11 }, tickLength: 8 } as unknown as never,
top: {} as unknown as never,
},
},
} as XYChartTheme;

renderHook( () => useChartMargin( 300, datedXOptions(), data, theme ) );

expect( mockGetEdgeTickWidths ).toHaveBeenCalledWith( tickValues, tickFormat, {
fontSize: '11px',
} );
} );

it( 'falls back to the raw tick label style when its font size is a relative unit', () => {
const theme = {
...baseTheme,
axisStyles: {
...baseTheme.axisStyles,
x: {
bottom: { tickLabel: { fontSize: '0.875rem' }, tickLength: 8 } as unknown as never,
top: {} as unknown as never,
},
},
} as XYChartTheme;

renderHook( () => useChartMargin( 300, datedXOptions(), data, theme ) );

expect( mockGetEdgeTickWidths ).toHaveBeenCalledWith( tickValues, tickFormat, {
fontSize: '0.875rem',
} );
} );

it( 'reserves the edge labels on a top x axis too', () => {
mockGetEdgeTickWidths.mockReturnValue( { first: 120, last: 60 } );

const { result } = renderHook( () =>
useChartMargin( 300, datedXOptions( { orientation: 'top' } ), data, baseTheme )
);

expect( result.current.right ).toBe( 30 );
expect( result.current.left ).toBe( 60 );
} );

it( 'reserves nothing for a hidden x axis', () => {
mockGetEdgeTickWidths.mockReturnValue( { first: 120, last: 60 } );

const { result } = renderHook( () =>
useChartMargin( 300, datedXOptions( { display: false } ), data, baseTheme )
);

expect( mockGetEdgeTickWidths ).not.toHaveBeenCalled();
expect( result.current.right ).toBe( 20 );
expect( result.current.left ).toBe( 51 );
} );
} );

describe( 'real measurement', () => {
// Everything else here mocks the measurer out; this block runs it for real,
// so that a width that never reaches the margin would fail something.
const actual = jest.requireActual( '../../utils/get-edge-tick-widths' );
type Measurable = { getComputedTextLength?: () => number };

afterEach( () => {
delete ( window.SVGElement.prototype as Measurable ).getComputedTextLength;
} );

it( 'turns a measured edge label into a reserved margin', () => {
// jsdom ships no getComputedTextLength, so @visx/text cannot measure at all.
( window.SVGElement.prototype as Measurable ).getComputedTextLength = function (
this: SVGElement
) {
return ( this.textContent ?? '' ).length * 8;
};
mockGetEdgeTickWidths.mockImplementation( actual.getEdgeTickWidths );

const options = {
...optionsBase,
axis: {
...optionsBase.axis,
x: {
tickValues: [ 1, 2 ],
tickFormat: ( _value: number, index: number ) =>
index === 0 ? 'AA' : 'MEASURED-LAST',
},
},
};

const { result } = renderHook( () => useChartMargin( 300, options, data, baseTheme ) );

// 'MEASURED-LAST' is 13 characters, so 104px wide, and half of it is reserved.
expect( result.current.right ).toBe( 52 );
} );
} );

describe( 'y axis gutter', () => {
it( 'measures a caller-pinned domain rather than the data range', () => {
const options = { ...optionsBase, yScale: { domain: [ 0, 1 ] as [ number, number ] } };

renderHook( () => useChartMargin( 300, options, data, baseTheme ) );

const ticks = mockGetLongestTickWidth.mock.calls[ 0 ][ 0 ] as number[];
expect( Math.max( ...ticks ) ).toBeLessThanOrEqual( 1 );
} );

it( 'reserves no gutter for a hidden y axis', () => {
const options = {
...optionsBase,
axis: { ...optionsBase.axis, y: { ...optionsBase.axis.y, display: false } },
};

const { result } = renderHook( () => useChartMargin( 300, options, data, baseTheme ) );

expect( result.current.left ).toBe( 20 );
} );
} );

describe( 'horizontal y ticks', () => {
const horizontalOptions = ( tickFormat: ( value: string | number ) => string ) => ( {
...optionsBase,
Expand Down
63 changes: 54 additions & 9 deletions projects/js-packages/charts/src/hooks/use-chart-margin.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { createScale, getTicks } from '@visx/scale';
import { useMemo } from 'react';
import { getLongestTickWidth, resolveFontSize } from '../utils';
import { getEdgeTickWidths, getLongestTickWidth, resolveFontSize } from '../utils';
import type { BaseChartProps, DataPointDate, SeriesData } from '../types';
import type { XYChartTheme } from '@visx/xychart';

Expand Down Expand Up @@ -50,6 +50,33 @@ const DEFAULT_TICK_LENGTH = 8;
*/
const DEFAULT_Y_TICK_WIDTH = 40;

type LabelStyle = { fontSize?: number | string; letterSpacing?: number | string };

/**
* Copy a label style with its lengths spelled out in px.
*
* `getStringWidth` applies the style through CSSOM. Blink resolves a bare number
* on an SVG `<text>`, but that is its own leniency rather than the CSS rule, and
* `buildChartTheme` hands us bare numbers.
*
* @param style - Raw label style from the theme.
* @return The same style with px-qualified lengths.
*/
const toMeasurableStyle = < T extends LabelStyle >( style?: T ) => {
if ( ! style ) {
return style;
}

const fontSize = resolveFontSize( style.fontSize );
const { letterSpacing } = style;

return {
...style,
...( fontSize === undefined ? {} : { fontSize: `${ fontSize }px` } ),
...( typeof letterSpacing === 'number' ? { letterSpacing: `${ letterSpacing }px` } : {} ),
};
};

const getXAxisLabelMetrics = ( theme: XYChartTheme, orientation: 'top' | 'bottom' ) => {
const xAxisStyles =
orientation === 'top' ? theme.axisStyles?.x?.top : theme.axisStyles?.x?.bottom;
Expand All @@ -61,7 +88,7 @@ const getXAxisLabelMetrics = ( theme: XYChartTheme, orientation: 'top' | 'bottom

const tickLength = xAxisStyles?.tickLength ?? DEFAULT_TICK_LENGTH;

return { fontSize, tickLength };
return { fontSize, tickLength, tickLabelStyle: toMeasurableStyle( xAxisStyles?.tickLabel ) };
};

export const useChartMargin = (
Expand All @@ -88,7 +115,9 @@ export const useChartMargin = (
const maxY = Math.max( ...allDataPoints.map( d => d.value ) );
const yScale = createScale( {
...options.yScale,
domain: [ minY, maxY ],
// A pinned domain is what the axis actually renders, so measure those
// ticks; the data's range would size the gutter for narrower labels.
domain: options.yScale?.domain ?? [ minY, maxY ],
range: [ height, 0 ],
} );

Expand All @@ -111,7 +140,7 @@ export const useChartMargin = (
const yTickWidth = getLongestTickWidth(
yTicks,
options.axis?.y?.tickFormat,
yAxisStyles.axisLabel
toMeasurableStyle( yAxisStyles.axisLabel )
);
// visx's default axis theme pushes y-axis tick labels a further 0.25em
// away from the axis (dx of -0.25em on the left, 0.25em on the right), so
Expand All @@ -125,17 +154,20 @@ export const useChartMargin = (
( yAxisStyles?.tickLength ?? 0 ) +
Math.ceil( yTickLabelFontSize * 0.25 );

if ( yAxisOrientation === 'right' ) {
defaultMargin.right = yMarginValue;
} else {
defaultMargin.left = yMarginValue;
// A hidden y axis reserves nothing; its gutter belongs to the plot area.
if ( options.axis?.y?.display !== false ) {
if ( yAxisOrientation === 'right' ) {
defaultMargin.right = yMarginValue;
} else {
defaultMargin.left = yMarginValue;
}
}

// Dynamically compute X-axis margin (bottom by default, or top if orientation is 'top').
// This mirrors Y-axis behavior where margin is based on label size and tick length,
// but keeps the padding minimal so consumers can control container spacing themselves.
const xOrientation = options.axis?.x?.orientation === 'top' ? 'top' : 'bottom';
const { fontSize, tickLength } = getXAxisLabelMetrics( theme, xOrientation );
const { fontSize, tickLength, tickLabelStyle } = getXAxisLabelMetrics( theme, xOrientation );
const computedXMargin = fontSize + tickLength;

if ( xOrientation === 'top' ) {
Expand All @@ -145,6 +177,19 @@ export const useChartMargin = (
defaultMargin.bottom = Math.max( defaultMargin.bottom, computedXMargin );
}

// An X-axis label is centered on its tick, so the ones at either end of the
// scale hang half their width outside the plot area and clip at the SVG edge.
if ( options.axis?.x?.display !== false ) {
const { first, last } = getEdgeTickWidths(
options.axis?.x?.tickValues ?? [],
options.axis?.x?.tickFormat,
tickLabelStyle
);

defaultMargin.left = Math.max( defaultMargin.left, Math.ceil( first / 2 ) );
defaultMargin.right = Math.max( defaultMargin.right, Math.ceil( last / 2 ) );
}

return defaultMargin;
}, [ options, theme, yTicks ] );
};
33 changes: 33 additions & 0 deletions projects/js-packages/charts/src/utils/get-edge-tick-widths.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { getStringWidth } from '@visx/text';
import type { TickFormatter } from '@visx/axis';
import type { AnyD3Scale, ScaleInput } from '@visx/scale';

/**
* Rendered widths of the first and last tick labels on an axis.
*
* An unmeasurable label reserves nothing, which is what a width of 0 already
* means to every caller, so it is reported as 0 rather than as its own case.
*
* @param ticks - Tick values, in axis order.
* @param formatTick - Function to format a tick.
* @param {object} labelStyle - Style object for the label.
* @return {object} - Widths in pixels.
*/
export const getEdgeTickWidths = < T extends AnyD3Scale >(
ticks: ScaleInput< T >[],
formatTick?: TickFormatter< ScaleInput< T > >,
labelStyle?: object
): { first: number; last: number } => {
if ( ! ticks.length ) {
return { first: 0, last: 0 };
}

const lastIndex = ticks.length - 1;
const label = ( tick: ScaleInput< T >, index: number ) =>
String( formatTick ? formatTick( tick, index, [] ) ?? '' : tick );

return {
first: getStringWidth( label( ticks[ 0 ], 0 ), labelStyle ) ?? 0,
last: getStringWidth( label( ticks[ lastIndex ], lastIndex ), labelStyle ) ?? 0,
};
};
Loading
Loading