From 332107f202ab0e99e742e0d6d6730a79417ba347 Mon Sep 17 00:00:00 2001 From: Sophia Mersmann Date: Thu, 19 Dec 2024 13:12:04 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=AA=20(line=20legend)=20add=20tests=20?= =?UTF-8?q?for=20dropping=20labels=20(#4309)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds tests in preparation for a refactor of the code responsible for dropping labels when there is not enough space. I also renamed some of the computed values in the line legend component for clarity. The SVG tester comes back with a difference because there was a small issue about label padding that I fixed. --- .../grapher/src/lineCharts/LineChart.tsx | 4 +- .../src/lineLegend/LineLegend.test.tsx | 204 ++++++++++++++++-- .../grapher/src/lineLegend/LineLegend.tsx | 102 +++++---- .../grapher/src/slopeCharts/SlopeChart.tsx | 16 +- .../src/stackedCharts/StackedAreaChart.tsx | 6 +- 5 files changed, 253 insertions(+), 79 deletions(-) diff --git a/packages/@ourworldindata/grapher/src/lineCharts/LineChart.tsx b/packages/@ourworldindata/grapher/src/lineCharts/LineChart.tsx index 516a4ddae56..dd54175a663 100644 --- a/packages/@ourworldindata/grapher/src/lineCharts/LineChart.tsx +++ b/packages/@ourworldindata/grapher/src/lineCharts/LineChart.tsx @@ -885,7 +885,7 @@ export class LineChart // only pass props that are required to calculate // the width to avoid circular dependencies return LineLegend.stableWidth({ - labelSeries: this.lineLegendSeries, + series: this.lineLegendSeries, maxWidth: this.maxLineLegendWidth, fontSize: this.fontSize, fontWeight: this.fontWeight, @@ -953,7 +953,7 @@ export class LineChart ))} {manager.showLegend && ( { + const yAxis = new AxisConfig({ min, max }).toVerticalAxis() + yAxis.range = yRange + return yAxis } +const makeSeries = ( + series: PartialBy[] +): LineLabelSeries[] => + series.map((s) => ({ + label: s.seriesName, + color: "blue", + ...s, + })) + +const series = makeSeries([ + { + seriesName: "Canada", + yValue: 50, + annotation: "A country in North America", + }, + { seriesName: "Mexico", yValue: 20, annotation: "Below Canada" }, +]) + it("can create a new legend", () => { - const legend = new LineLegend(props) + const legend = new LineLegend({ + series, + yAxis: makeAxis({ yRange: [0, 100] }), + }) + + expect(legend.visibleSeriesNames.length).toEqual(2) +}) + +describe("dropping labels", () => { + it("drops labels that don't fit into the available space", () => { + const lineLegend = new LineLegend({ + series, + yAxis: makeAxis({ yRange: [0, 50] }), + }) + + // two labels are given, but only one fits + expect(lineLegend.sizedSeries).toHaveLength(2) + expect(lineLegend.visibleSeriesNames).toEqual(["Canada"]) + }) + + it("prioritises labels based on importance sorting", () => { + const lineLegend = new LineLegend({ + series, + yAxis: makeAxis({ yRange: [0, 50] }), + seriesNamesSortedByImportance: ["Mexico", "Canada"], + }) + + // 'Mexico' is picked since it's given higher importance + expect(lineLegend.visibleSeriesNames).toEqual(["Mexico"]) + }) + + it("skips more important series if they don't fit", () => { + const series = makeSeries([ + { seriesName: "Canada", yValue: 5 }, + { seriesName: "Mexico", yValue: 20 }, + { seriesName: "Spain", yValue: 40 }, + { seriesName: "Democratic Republic of Congo", yValue: 45 }, + ]) + + const lineLegend = new LineLegend({ + series, + yAxis: makeAxis({ yRange: [0, 50] }), + maxWidth: 100, + seriesNamesSortedByImportance: [ + "Mexico", + "Canada", + "Democratic Republic of Congo", + "Spain", + ], + }) + + // 'Democratic Republic of Congo' is skipped since it doesn't fit + expect(lineLegend.visibleSeriesNames).toEqual([ + "Mexico", + "Canada", + "Spain", + ]) + }) + + it("prioritises to label focused series", () => { + const seriesWithFocus = series.map((s) => ({ + ...s, + focus: { + active: s.seriesName === "Mexico", + background: s.seriesName !== "Mexico", + }, + })) + + const lineLegendWithFocus = new LineLegend({ + series: seriesWithFocus, + yAxis: makeAxis({ yRange: [0, 50] }), + }) + + // 'Mexico' is picked since it's focused + expect(lineLegendWithFocus.visibleSeriesNames).toEqual(["Mexico"]) + }) + + it("uses all available space", () => { + const series = makeSeries([ + { seriesName: "Canada", yValue: 5 }, + { seriesName: "Mexico", yValue: 20 }, + { seriesName: "Spain", yValue: 40 }, + { seriesName: "France", yValue: 45 }, + ]) + + const yRange: [number, number] = [0, 50] + const lineLegend = new LineLegend({ + series, + yAxis: makeAxis({ yRange }), + }) + + // 'Spain' is dropped since it doesn't fit + expect(lineLegend.visibleSeriesNames).toEqual([ + "Canada", + "Mexico", + "France", + ]) + + // verify that we can't fit 'Spain' into the available space + const droppedLabel = lineLegend.sizedSeries.find( + (series) => series.seriesName === "Spain" + )! + const droppedLabelHeight = droppedLabel.height + LEGEND_ITEM_MIN_SPACING + const availableHeight = yRange[1] - yRange[0] + const remainingHeight = availableHeight - lineLegend.visibleSeriesHeight + expect(remainingHeight).toBeLessThan(droppedLabelHeight) + }) + + it("picks labels from the edges", () => { + const series = makeSeries([ + { seriesName: "Canada", yValue: 10 }, + { seriesName: "Mexico", yValue: 50 }, + { seriesName: "France", yValue: 90 }, + ]) + + const lineLegend = new LineLegend({ + series, + yAxis: makeAxis({ yRange: [0, 40] }), + }) + + expect(lineLegend.visibleSeriesNames).toEqual(["Canada", "France"]) + }) + + it("picks labels in a balanced way", () => { + const series = makeSeries([ + { seriesName: "Canada", yValue: 10 }, + { seriesName: "Mexico", yValue: 12 }, + { seriesName: "Brazil", yValue: 14 }, + { seriesName: "Argentina", yValue: 15 }, + { seriesName: "Chile", yValue: 60 }, + { seriesName: "Peru", yValue: 90 }, + ]) + + const lineLegend = new LineLegend({ + series, + yAxis: makeAxis({ yRange: [0, 50] }), + }) - expect(legend.sizedLabels.length).toEqual(2) + // drops 'Mexico', 'Brazil' and 'Argentina' since they're close to each other + expect(lineLegend.visibleSeriesNames).toEqual([ + "Canada", + "Chile", + "Peru", + ]) + }) }) diff --git a/packages/@ourworldindata/grapher/src/lineLegend/LineLegend.tsx b/packages/@ourworldindata/grapher/src/lineLegend/LineLegend.tsx index 33c84a512e3..fef2260f4a9 100644 --- a/packages/@ourworldindata/grapher/src/lineLegend/LineLegend.tsx +++ b/packages/@ourworldindata/grapher/src/lineLegend/LineLegend.tsx @@ -39,7 +39,7 @@ import { // text color for labels of background series const NON_FOCUSED_TEXT_COLOR = GRAY_70 // Minimum vertical space between two legend items -const LEGEND_ITEM_MIN_SPACING = 4 +export const LEGEND_ITEM_MIN_SPACING = 4 // Horizontal distance from the end of the chart to the start of the marker const MARKER_MARGIN = 4 // Space between the label and the annotation @@ -319,7 +319,7 @@ class LineLabels extends React.Component<{ } export interface LineLegendProps { - labelSeries: LineLabelSeries[] + series: LineLabelSeries[] yAxis?: VerticalAxis // positioning @@ -336,7 +336,7 @@ export interface LineLegendProps { textOutlineColor?: Color // used to determine which series should be labelled when there is limited space - seriesSortedByImportance?: SeriesName[] + seriesNamesSortedByImportance?: SeriesName[] // interactions isStatic?: boolean // don't add interactions if true @@ -453,11 +453,11 @@ export class LineLegend extends React.Component { }) } - @computed.struct get sizedLabels(): SizedSeries[] { + @computed.struct get sizedSeries(): SizedSeries[] { const { fontWeight: globalFontWeight } = this - return this.props.labelSeries.map((label) => { - const textWrap = this.makeLabelTextWrap(label) - const annotationTextWrap = this.makeAnnotationTextWrap(label) + return this.props.series.map((series) => { + const textWrap = this.makeLabelTextWrap(series) + const annotationTextWrap = this.makeAnnotationTextWrap(series) const annotationWidth = annotationTextWrap?.width ?? 0 const annotationHeight = annotationTextWrap @@ -466,13 +466,13 @@ export class LineLegend extends React.Component { // font weight priority: // series focus state > presense of value label > globally set font weight - const activeFontWeight = label.focus?.active ? 700 : undefined - const seriesFontWeight = label.formattedValue ? 700 : undefined + const activeFontWeight = series.focus?.active ? 700 : undefined + const seriesFontWeight = series.formattedValue ? 700 : undefined const fontWeight = activeFontWeight ?? seriesFontWeight ?? globalFontWeight return { - ...label, + ...series, textWrap, annotationTextWrap, width: Math.max(textWrap.width, annotationWidth), @@ -483,8 +483,8 @@ export class LineLegend extends React.Component { } @computed private get maxLabelWidth(): number { - const { sizedLabels = [] } = this - return max(sizedLabels.map((d) => d.width)) ?? 0 + const { sizedSeries = [] } = this + return max(sizedSeries.map((d) => d.width)) ?? 0 } @computed get stableWidth(): number { @@ -530,25 +530,25 @@ export class LineLegend extends React.Component { } // Naive initial placement of each mark at the target height, before collision detection - @computed private get initialSeries(): PlacedSeries[] { + @computed private get initialPlacedSeries(): PlacedSeries[] { const { yAxis, legendX, legendY } = this const [legendYMin, legendYMax] = legendY - return this.sizedLabels.map((label) => { - const labelHeight = label.height - const labelWidth = label.width + DEFAULT_CONNECTOR_LINE_WIDTH + return this.sizedSeries.map((series) => { + const labelHeight = series.height + const labelWidth = series.width + DEFAULT_CONNECTOR_LINE_WIDTH - const midY = yAxis.place(label.yValue) + const midY = yAxis.place(series.yValue) const origBounds = new Bounds( legendX, - midY - label.height / 2, + midY - series.height / 2, labelWidth, labelHeight ) // ensure label doesn't go beyond the top or bottom of the chart - const initialY = this.getYPositionForSeriesLabel(label) + const initialY = this.getYPositionForSeriesLabel(series) const y = Math.min( Math.max(initialY, legendYMin), legendYMax - labelHeight @@ -556,7 +556,7 @@ export class LineLegend extends React.Component { const bounds = new Bounds(legendX, y, labelWidth, labelHeight) return { - ...label, + ...series, y, midY, origBounds, @@ -568,15 +568,18 @@ export class LineLegend extends React.Component { }) } - @computed get initialSeriesByName(): Map { - return new Map(this.initialSeries.map((d) => [d.seriesName, d])) + @computed get initialPlacedSeriesByName(): Map { + return new Map(this.initialPlacedSeries.map((d) => [d.seriesName, d])) } @computed get placedSeries(): PlacedSeries[] { const [yLegendMin, yLegendMax] = this.legendY // ensure list is sorted by the visual position in ascending order - const sortedSeries = sortBy(this.visibleSeries, (label) => label.midY) + const sortedSeries = sortBy( + this.visiblePlacedSeries, + (label) => label.midY + ) const groups: PlacedSeries[][] = cloneDeep(sortedSeries).map((mark) => [ mark, @@ -644,37 +647,45 @@ export class LineLegend extends React.Component { return groups.flat() } - @computed get sortedSeriesByImportance(): PlacedSeries[] | undefined { - if (!this.props.seriesSortedByImportance) return undefined + @computed get seriesSortedByImportance(): PlacedSeries[] | undefined { + if (!this.props.seriesNamesSortedByImportance) return undefined return excludeUndefined( - this.props.seriesSortedByImportance.map((seriesName) => - this.initialSeriesByName.get(seriesName) + this.props.seriesNamesSortedByImportance.map((seriesName) => + this.initialPlacedSeriesByName.get(seriesName) ) ) } - @computed get visibleSeries(): PlacedSeries[] { + private computeHeight(series: PlacedSeries[]): number { + return ( + sumBy(series, (series) => series.bounds.height) + + (series.length - 1) * LEGEND_ITEM_MIN_SPACING + ) + } + + @computed get visiblePlacedSeries(): PlacedSeries[] { const { legendY } = this const availableHeight = Math.abs(legendY[1] - legendY[0]) - const nonOverlappingMinHeight = - sumBy(this.initialSeries, (series) => series.bounds.height) + - this.initialSeries.length * LEGEND_ITEM_MIN_SPACING + const nonOverlappingMinHeight = this.computeHeight( + this.initialPlacedSeries + ) // early return if filtering is not needed if (nonOverlappingMinHeight <= availableHeight) - return this.initialSeries + return this.initialPlacedSeries - if (this.sortedSeriesByImportance) { + if (this.seriesSortedByImportance) { // keep a subset of series that fit within the available height, // prioritizing by importance. Note that more important (but longer) // series names are skipped if they don't fit. const keepSeries: PlacedSeries[] = [] let keepSeriesHeight = 0 - for (const series of this.sortedSeriesByImportance) { + for (const series of this.seriesSortedByImportance) { + // if the candidate is the first one, don't add padding + const padding = + keepSeries.length === 0 ? 0 : LEGEND_ITEM_MIN_SPACING const newHeight = - keepSeriesHeight + - series.bounds.height + - LEGEND_ITEM_MIN_SPACING + keepSeriesHeight + series.bounds.height + padding if (newHeight <= availableHeight) { keepSeries.push(series) keepSeriesHeight = newHeight @@ -683,16 +694,17 @@ export class LineLegend extends React.Component { } return keepSeries } else { - const candidates = new Set(this.initialSeries) + const candidates = new Set(this.initialPlacedSeries) const sortedKeepSeries: PlacedSeries[] = [] let keepSeriesHeight = 0 const maybePickCandidate = (candidate: PlacedSeries): boolean => { + // if the candidate is the first one, don't add padding + const padding = + sortedKeepSeries.length === 0 ? 0 : LEGEND_ITEM_MIN_SPACING const newHeight = - keepSeriesHeight + - candidate.bounds.height + - LEGEND_ITEM_MIN_SPACING + keepSeriesHeight + candidate.bounds.height + padding if (newHeight <= availableHeight) { const insertIndex = sortedIndexBy( sortedKeepSeries, @@ -728,7 +740,7 @@ export class LineLegend extends React.Component { } const [focusedCandidates, nonFocusedCandidates] = partition( - this.initialSeries, + this.initialPlacedSeries, (series) => series.focus?.active ) @@ -844,7 +856,11 @@ export class LineLegend extends React.Component { } @computed get visibleSeriesNames(): SeriesName[] { - return this.visibleSeries.map((series) => series.seriesName) + return this.visiblePlacedSeries.map((series) => series.seriesName) + } + + @computed get visibleSeriesHeight(): number { + return this.computeHeight(this.visiblePlacedSeries) } // Does this placement need line markers or is the position of the labels already clear? diff --git a/packages/@ourworldindata/grapher/src/slopeCharts/SlopeChart.tsx b/packages/@ourworldindata/grapher/src/slopeCharts/SlopeChart.tsx index 8b4428e1b5e..fe4e70e094d 100644 --- a/packages/@ourworldindata/grapher/src/slopeCharts/SlopeChart.tsx +++ b/packages/@ourworldindata/grapher/src/slopeCharts/SlopeChart.tsx @@ -651,7 +651,7 @@ export class SlopeChart @computed private get lineLegendPropsLeft(): Partial { return { xAnchor: "end", - seriesSortedByImportance: + seriesNamesSortedByImportance: this.seriesSortedByImportanceForLineLegendLeft, } } @@ -673,17 +673,17 @@ export class SlopeChart ) return LineLegend.maxLevel({ - labelSeries: series, + series, ...this.lineLegendPropsCommon, - seriesSortedByImportance: + seriesNamesSortedByImportance: this.seriesSortedByImportanceForLineLegendLeft, // not including `lineLegendPropsLeft` due to a circular dependency }) } @computed get lineLegendWidthLeft(): number { - const props = { - labelSeries: this.lineLegendSeriesLeft, + const props: LineLegendProps = { + series: this.lineLegendSeriesLeft, ...this.lineLegendPropsCommon, ...this.lineLegendPropsLeft, } @@ -702,7 +702,7 @@ export class SlopeChart @computed get lineLegendRight(): LineLegend { return new LineLegend({ - labelSeries: this.lineLegendSeriesRight, + series: this.lineLegendSeriesRight, ...this.lineLegendPropsCommon, ...this.lineLegendPropsRight, }) @@ -1221,7 +1221,7 @@ export class SlopeChart private renderLineLegendRight(): React.ReactElement { return (