From ac1dca224a8214ee0346676289d0b244abf7ea2f Mon Sep 17 00:00:00 2001 From: Sophia Mersmann Date: Tue, 2 Jul 2024 15:39:32 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=89=20(stacked=20bar)=20share=20x-axis?= =?UTF-8?q?=20across=20facets=20(#3733)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../@ourworldindata/grapher/src/axis/Axis.ts | 8 ++ .../grapher/src/axis/AxisConfig.ts | 2 + .../grapher/src/axis/AxisViews.tsx | 134 ++++++++++-------- .../grapher/src/facetChart/FacetChart.tsx | 77 ++++++---- .../src/facetChart/FacetChartUtils.tsx | 18 ++- .../src/stackedCharts/StackedBarChart.tsx | 6 +- .../types/src/domainTypes/Layout.ts | 1 + .../types/src/grapherTypes/GrapherTypes.ts | 1 + .../@ourworldindata/utils/src/Bounds.test.ts | 46 +++++- packages/@ourworldindata/utils/src/Bounds.ts | 31 ++-- packages/@ourworldindata/utils/src/Util.ts | 3 +- 11 files changed, 218 insertions(+), 109 deletions(-) diff --git a/packages/@ourworldindata/grapher/src/axis/Axis.ts b/packages/@ourworldindata/grapher/src/axis/Axis.ts index 5f1870a1a46..13333af6841 100644 --- a/packages/@ourworldindata/grapher/src/axis/Axis.ts +++ b/packages/@ourworldindata/grapher/src/axis/Axis.ts @@ -520,6 +520,10 @@ export class HorizontalAxis extends AbstractAxis { return this.rangeSize } + // note that we intentionally don't take `hideAxisLabels` into account here. + // tick labels might be hidden in faceted charts. when faceted, it's important + // the axis size doesn't change as a result of hiding the axis labels, or else + // we might end up with misaligned axes. @computed get height(): number { if (this.hideAxis) return 0 const { labelOffset, labelPadding } = this @@ -635,6 +639,10 @@ export class VerticalAxis extends AbstractAxis { : 0 } + // note that we intentionally don't take `hideAxisLabels` into account here. + // tick labels might be hidden in faceted charts. when faceted, it's important + // the axis size doesn't change as a result of hiding the axis labels, or else + // we might end up with misaligned axes. @computed get width(): number { if (this.hideAxis) return 0 const { labelOffset, labelPadding } = this diff --git a/packages/@ourworldindata/grapher/src/axis/AxisConfig.ts b/packages/@ourworldindata/grapher/src/axis/AxisConfig.ts index cdc8d6486cd..ff206724cdf 100644 --- a/packages/@ourworldindata/grapher/src/axis/AxisConfig.ts +++ b/packages/@ourworldindata/grapher/src/axis/AxisConfig.ts @@ -31,6 +31,7 @@ class AxisConfigDefaults implements AxisConfigInterface { @observable.ref minSize?: number = undefined @observable.ref hideAxis?: boolean = undefined @observable.ref hideGridlines?: boolean = undefined + @observable.ref hideTickLabels?: boolean = undefined @observable.ref labelPadding?: number = undefined @observable.ref nice?: boolean = undefined @observable.ref maxTicks?: number = undefined @@ -70,6 +71,7 @@ export class AxisConfig minSize: this.minSize, hideAxis: this.hideAxis, hideGridlines: this.hideGridlines, + hideTickLabels: this.hideTickLabels, labelPadding: this.labelPadding, nice: this.nice, maxTicks: this.maxTicks, diff --git a/packages/@ourworldindata/grapher/src/axis/AxisViews.tsx b/packages/@ourworldindata/grapher/src/axis/AxisViews.tsx index c3407b03d75..20208efac60 100644 --- a/packages/@ourworldindata/grapher/src/axis/AxisViews.tsx +++ b/packages/@ourworldindata/grapher/src/axis/AxisViews.tsx @@ -260,7 +260,7 @@ export class VerticalAxisComponent extends React.Component<{ detailsMarker, showTickMarks, } = this.props - const { tickLabels, labelTextWrap } = verticalAxis + const { tickLabels, labelTextWrap, config } = verticalAxis return ( )} - - {tickLabels.map((label, i) => { - const { y, xAlign, yAlign, formattedValue } = label - return ( - - {formattedValue} - - ) - })} - + {!config.hideTickLabels && ( + + {tickLabels.map((label, i) => { + const { y, xAlign, yAlign, formattedValue } = label + return ( + + {formattedValue} + + ) + })} + + )} ) } @@ -384,6 +388,8 @@ export class HorizontalAxisComponent extends React.Component<{ ? bounds.top + labelOffset + 10 : bounds.bottom - labelOffset + const showTickLabels = !axis.config.hideTickLabels + return ( { - const { x, xAlign, formattedValue } = label - return ( - - {showTickMarks && ( - - )} - { + const { x, xAlign, formattedValue } = label + return ( + - {formattedValue} - - - ) - })} + {showTickMarks && ( + + )} + {showTickLabels && ( + + {formattedValue} + + )} + + ) + })} ) } diff --git a/packages/@ourworldindata/grapher/src/facetChart/FacetChart.tsx b/packages/@ourworldindata/grapher/src/facetChart/FacetChart.tsx index 35ce446c21e..914a9d915fd 100644 --- a/packages/@ourworldindata/grapher/src/facetChart/FacetChart.tsx +++ b/packages/@ourworldindata/grapher/src/facetChart/FacetChart.tsx @@ -65,6 +65,8 @@ import { NumericBin, } from "../color/ColorScaleBin" +const SHARED_X_AXIS_MIN_FACET_COUNT = 12 + const facetBackgroundColor = "none" // we don't use color yet but may use it for background later const getContentBounds = ( @@ -211,7 +213,11 @@ export class FacetChart columnPadding: number outerPadding: number } { - return getChartPadding(this.facetFontSize) + const { isSharedXAxis, facetFontSize } = this + return getChartPadding({ + baseFontSize: facetFontSize, + isSharedXAxis, + }) } @computed private get hideFacetLegends(): boolean { @@ -342,6 +348,29 @@ export class FacetChart }) } + @computed private get isSharedYAxis(): boolean { + // When the Y axis is uniform for all facets: + // - for most charts, we want to only show the axis on the left-most facet charts, and omit + // it on the others + // - for bar charts the Y axis is plotted horizontally, so we don't want to omit it + return ( + this.uniformYAxis && + ![ + ChartTypeName.StackedDiscreteBar, + ChartTypeName.DiscreteBar, + ].includes(this.chartTypeName) + ) + } + + @computed private get isSharedXAxis(): boolean { + return ( + this.uniformXAxis && + // TODO: do this for stacked area charts and line charts as well? + this.chartTypeName === ChartTypeName.StackedBar && + this.facetCount >= SHARED_X_AXIS_MIN_FACET_COUNT + ) + } + // Only made public for testing @computed get placedSeries(): PlacedFacetSeries[] { const { intermediateChartInstances } = this @@ -355,36 +384,18 @@ export class FacetChart // Define the global axis config, shared between all facets const sharedAxesSizes: PositionMap = {} - // When the Y axis is uniform for all facets: - // - for most charts, we want to only show the axis on the left-most facet charts, and omit - // it on the others - // - for bar charts the Y axis is plotted horizontally, so we don't want to omit it - const isSharedYAxis = - this.uniformYAxis && - ![ - ChartTypeName.StackedDiscreteBar, - ChartTypeName.DiscreteBar, - ].includes(this.chartTypeName) - const axes: AxesInfo = { x: { config: {}, axisAccessor: (instance) => instance.xAxis, uniform: this.uniformXAxis, - // For now, X axes are never shared for any chart. - // If we ever allow them to be shared, we need to be careful with how we determine - // the `minSize` – in the intermediate series (at this time) all axes are shown in - // order to find the one with maximum size, but in the placed series, some axes are - // hidden. This expands the available area for the chart, which can in turn increase - // the number of ticks shown, which can make the size of the axis in the placed - // series greater than the one in the intermediate series. - shared: false, + shared: this.isSharedXAxis, }, y: { config: {}, axisAccessor: (instance) => instance.yAxis, uniform: this.uniformYAxis, - shared: isSharedYAxis, + shared: this.isSharedYAxis, }, } values(axes).forEach(({ config, axisAccessor, uniform, shared }) => { @@ -425,7 +436,11 @@ export class FacetChart config.max, ]) config.minSize = size - if (shared) sharedAxesSizes[axis.orient] = size + if (shared) { + const sharedAxisSize = + axis.orient === Position.bottom ? 0 : size + sharedAxesSizes[axis.orient] = sharedAxisSize + } } } else if (axisWithMaxSize) { config.minSize = axisWithMaxSize.size @@ -446,10 +461,10 @@ export class FacetChart return this.intermediatePlacedSeries.map((series, i) => { const chartInstance = intermediateChartInstances[i] const { xAxis, yAxis } = chartInstance - const { bounds: initialGridBounds, edges } = gridBoundsArr[i] + const { bounds: initialGridBounds, cellEdges } = gridBoundsArr[i] let bounds = initialGridBounds // Only expand bounds if the facet is on the same edge as the shared axes - for (const edge of edges) { + for (const edge of cellEdges) { bounds = bounds.expand({ [edge]: sharedAxesSizes[edge], }) @@ -461,9 +476,17 @@ export class FacetChart useValueBasedColorScheme, externalLegendFocusBin: this.legendFocusBin, xAxisConfig: { - hideAxis: shouldHideFacetAxis( + // For now, sharing an x axis means hiding the tick labels of inner facets. + // This means that none of the x axes are actually hidden (we just don't plot their tick labels). + // If we ever allow shared x axes to be actually hidden, we need to be careful with how we determine + // the `minSize` – in the intermediate series (at this time) all axes are shown in + // order to find the one with maximum size, but in the placed series, some axes are + // hidden. This expands the available area for the chart, which can in turn increase + // the number of ticks shown, which can make the size of the axis in the placed + // series greater than the one in the intermediate series. + hideTickLabels: shouldHideFacetAxis( xAxis, - edges, + cellEdges, sharedAxesSizes ), ...series.manager.xAxisConfig, @@ -472,7 +495,7 @@ export class FacetChart yAxisConfig: { hideAxis: shouldHideFacetAxis( yAxis, - edges, + cellEdges, sharedAxesSizes ), ...series.manager.yAxisConfig, diff --git a/packages/@ourworldindata/grapher/src/facetChart/FacetChartUtils.tsx b/packages/@ourworldindata/grapher/src/facetChart/FacetChartUtils.tsx index 354bf3e543f..d85771d36a7 100644 --- a/packages/@ourworldindata/grapher/src/facetChart/FacetChartUtils.tsx +++ b/packages/@ourworldindata/grapher/src/facetChart/FacetChartUtils.tsx @@ -17,14 +17,24 @@ export const getFontSize = ( export const getLabelPadding = (baseFontSize: number): number => 0.5 * baseFontSize -export const getChartPadding = ( +export const getChartPadding = ({ + baseFontSize, + isSharedXAxis, +}: { baseFontSize: number -): { rowPadding: number; columnPadding: number; outerPadding: number } => { + isSharedXAxis: boolean +}): { rowPadding: number; columnPadding: number; outerPadding: number } => { const labelHeight = baseFontSize const labelPadding = getLabelPadding(baseFontSize) + + const rowPadding = isSharedXAxis ? 0 : 1 + const columnPadding = 1 + return { - rowPadding: Math.round(labelHeight + labelPadding + baseFontSize), - columnPadding: Math.round(baseFontSize), + rowPadding: Math.round( + labelHeight + labelPadding + rowPadding * baseFontSize + ), + columnPadding: Math.round(columnPadding * baseFontSize), outerPadding: 0, } } diff --git a/packages/@ourworldindata/grapher/src/stackedCharts/StackedBarChart.tsx b/packages/@ourworldindata/grapher/src/stackedCharts/StackedBarChart.tsx index 497b1d5624f..0781ef3209e 100644 --- a/packages/@ourworldindata/grapher/src/stackedCharts/StackedBarChart.tsx +++ b/packages/@ourworldindata/grapher/src/stackedCharts/StackedBarChart.tsx @@ -192,7 +192,11 @@ export class StackedBarChart } @computed protected get paddingForLegendRight(): number { - return this.showHorizontalLegend ? 0 : this.sidebarWidth + 20 + return this.showHorizontalLegend + ? 0 + : this.sidebarWidth > 0 + ? this.sidebarWidth + 20 + : 0 } @computed protected get paddingForLegendTop(): number { diff --git a/packages/@ourworldindata/types/src/domainTypes/Layout.ts b/packages/@ourworldindata/types/src/domainTypes/Layout.ts index ec2eba1a293..1f6f7d91602 100644 --- a/packages/@ourworldindata/types/src/domainTypes/Layout.ts +++ b/packages/@ourworldindata/types/src/domainTypes/Layout.ts @@ -30,4 +30,5 @@ export enum AxisAlign { export interface GridParameters { rows: number columns: number + count: number } diff --git a/packages/@ourworldindata/types/src/grapherTypes/GrapherTypes.ts b/packages/@ourworldindata/types/src/grapherTypes/GrapherTypes.ts index 6468bb58137..0d067710c0d 100644 --- a/packages/@ourworldindata/types/src/grapherTypes/GrapherTypes.ts +++ b/packages/@ourworldindata/types/src/grapherTypes/GrapherTypes.ts @@ -226,6 +226,7 @@ export interface AxisConfigInterface { canChangeScaleType?: boolean removePointsOutsideDomain?: boolean hideAxis?: boolean + hideTickLabels?: boolean /** Hide the faint lines that are shown inside the plot (axis ticks may still be visible). */ hideGridlines?: boolean diff --git a/packages/@ourworldindata/utils/src/Bounds.test.ts b/packages/@ourworldindata/utils/src/Bounds.test.ts index 9f26b1fd604..a37cdb33457 100755 --- a/packages/@ourworldindata/utils/src/Bounds.test.ts +++ b/packages/@ourworldindata/utils/src/Bounds.test.ts @@ -10,7 +10,7 @@ it("can report the center", () => { it("can split a bounds into correct number of pieces", () => { const bounds = new Bounds(0, 0, 100, 100) - const quads = bounds.grid({ rows: 2, columns: 2 }) + const quads = bounds.grid({ rows: 2, columns: 2, count: 4 }) expect(quads.length).toEqual(4) const first = quads[0] const second = quads[1] @@ -22,24 +22,58 @@ it("can split a bounds into correct number of pieces", () => { expect(third.bounds.x).toEqual(0) expect(fourth.bounds.height).toEqual(50) - expect(Array.from(first.edges.values())).toEqual( + expect(Array.from(first.cellEdges.values())).toEqual( expect.arrayContaining([Position.left, Position.top]) ) - expect(Array.from(second.edges.values())).toEqual( + expect(Array.from(second.cellEdges.values())).toEqual( expect.arrayContaining([Position.top, Position.right]) ) - expect(Array.from(third.edges.values())).toEqual( + expect(Array.from(third.cellEdges.values())).toEqual( expect.arrayContaining([Position.left, Position.bottom]) ) - expect(Array.from(fourth.edges.values())).toEqual( + expect(Array.from(fourth.cellEdges.values())).toEqual( expect.arrayContaining([Position.bottom, Position.right]) ) }) +it("can split a bounds into an arbitrary number of pieces", () => { + const bounds = new Bounds(0, 0, 100, 100) + const grid = bounds.grid({ rows: 2, columns: 4, count: 5 }) + expect(grid.length).toEqual(5) + const first = grid[0] + const second = grid[1] + const third = grid[2] + const fourth = grid[3] + const fifth = grid[4] + + expect(second.bounds.x).toEqual(25) + expect(third.bounds.y).toEqual(0) + expect(third.bounds.x).toEqual(50) + expect(fourth.bounds.height).toEqual(50) + expect(fifth.bounds.x).toEqual(0) + expect(fifth.bounds.y).toEqual(50) + + expect(Array.from(first.cellEdges.values())).toEqual( + expect.arrayContaining([Position.left, Position.top]) + ) + expect(Array.from(second.cellEdges.values())).toEqual( + expect.arrayContaining([Position.top, Position.bottom]) + ) + expect(Array.from(third.cellEdges.values())).toEqual( + expect.arrayContaining([Position.top, Position.bottom]) + ) + expect(Array.from(fourth.cellEdges.values())).toEqual( + expect.arrayContaining([Position.top, Position.right, Position.bottom]) + ) + expect(Array.from(fifth.cellEdges.values())).toEqual( + expect.arrayContaining([Position.right, Position.bottom, Position.left]) + ) +}) + it("can split with padding between charts", () => { const bounds = new Bounds(10, 10, 100, 100) const quads = bounds.grid( - { rows: 2, columns: 2 }, + { rows: 2, columns: 2, count: 4 }, { rowPadding: 20, columnPadding: 20 } ) expect(quads.length).toEqual(4) diff --git a/packages/@ourworldindata/utils/src/Bounds.ts b/packages/@ourworldindata/utils/src/Bounds.ts index 1ff82ee5a2b..3197a2e13ca 100644 --- a/packages/@ourworldindata/utils/src/Bounds.ts +++ b/packages/@ourworldindata/utils/src/Bounds.ts @@ -18,7 +18,7 @@ type PadObject = PositionMap export interface GridBounds { col: number row: number - edges: Set + cellEdges: Set bounds: Bounds } @@ -359,7 +359,7 @@ export class Bounds { gridParams: GridParameters, padding: SplitBoundsPadding = {} ): GridBounds[] { - const { columns, rows } = gridParams + const { columns, rows, count } = gridParams const { columnPadding = 0, rowPadding = 0, outerPadding = 0 } = padding const contentWidth = @@ -369,18 +369,29 @@ export class Bounds { const boxWidth = Math.floor(contentWidth / columns) const boxHeight = Math.floor(contentHeight / rows) - return range(0, rows * columns).map((index: number) => { + const hasCell = (row: number, col: number): boolean => { + if (row < 0 || row >= rows) return false + if (col < 0 || col >= columns) return false + const index = row * columns + col + if (index >= count) return false + return true + } + + const grid = range(0, count).map((index: number) => { const col = index % columns const row = Math.floor(index / columns) - const edges = new Set() - if (col === 0) edges.add(Position.left) - if (col === columns - 1) edges.add(Position.right) - if (row === 0) edges.add(Position.top) - if (row === rows - 1) edges.add(Position.bottom) + + // edges around the existing cells + const cellEdges = new Set() + if (!hasCell(row, col - 1)) cellEdges.add(Position.left) + if (!hasCell(row, col + 1)) cellEdges.add(Position.right) + if (!hasCell(row - 1, col)) cellEdges.add(Position.top) + if (!hasCell(row + 1, col)) cellEdges.add(Position.bottom) + return { row, col, - edges, + cellEdges, bounds: new Bounds( this.x + outerPadding + col * (boxWidth + columnPadding), this.y + outerPadding + row * (boxHeight + rowPadding), @@ -389,6 +400,8 @@ export class Bounds { ), } }) + + return grid } yRange(): [number, number] { diff --git a/packages/@ourworldindata/utils/src/Util.ts b/packages/@ourworldindata/utils/src/Util.ts index 0d917a65733..567ba543efe 100644 --- a/packages/@ourworldindata/utils/src/Util.ts +++ b/packages/@ourworldindata/utils/src/Util.ts @@ -652,7 +652,7 @@ export const getIdealGridParams = ({ const ratio = containerAspectRatio / idealAspectRatio // Prefer vertical grid for count=2. if (count === 2 && containerAspectRatio < 2.8) - return { rows: 2, columns: 1 } + return { rows: 2, columns: 1, count } // Otherwise, optimize for closest to the ideal aspect ratio. const initialColumns = Math.min(Math.round(Math.sqrt(count * ratio)), count) const rows = Math.ceil(count / initialColumns) @@ -662,6 +662,7 @@ export const getIdealGridParams = ({ return { rows, columns, + count, } }