From f5bf42116b50d1d49270ec9bbb8a64e721c81220 Mon Sep 17 00:00:00 2001 From: Sophia Mersmann Date: Fri, 10 Jan 2025 14:18:05 +0100 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=8E=89=20(line)=20allow=20to=20discon?= =?UTF-8?q?nect=20lines=20small=20change?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- adminSiteClient/DimensionCard.tsx | 12 ++++ .../grapher/src/lineCharts/LineChart.tsx | 69 +++++++++++++++---- .../src/lineCharts/LineChartConstants.ts | 1 + .../src/schema/grapher-schema.006.yaml | 5 ++ .../src/OwidVariableDisplayConfigInterface.ts | 1 + .../@ourworldindata/utils/src/OwidVariable.ts | 1 + 6 files changed, 74 insertions(+), 15 deletions(-) diff --git a/adminSiteClient/DimensionCard.tsx b/adminSiteClient/DimensionCard.tsx index db3d094a02c..d812d2ba16e 100644 --- a/adminSiteClient/DimensionCard.tsx +++ b/adminSiteClient/DimensionCard.tsx @@ -51,6 +51,11 @@ export class DimensionCard< this.onChange() } + @action.bound onIsConnected(value: boolean) { + this.props.dimension.display.isConnected = value + this.onChange() + } + @action.bound onColor(color: string | undefined) { this.props.dimension.display.color = color this.onChange() @@ -252,6 +257,13 @@ export class DimensionCard< onValue={this.onIsProjection} /> )} + {grapher.isLineChart && ( + + )}
)} diff --git a/packages/@ourworldindata/grapher/src/lineCharts/LineChart.tsx b/packages/@ourworldindata/grapher/src/lineCharts/LineChart.tsx index 3369bbbfa26..5033d637d3d 100644 --- a/packages/@ourworldindata/grapher/src/lineCharts/LineChart.tsx +++ b/packages/@ourworldindata/grapher/src/lineCharts/LineChart.tsx @@ -127,6 +127,7 @@ const VARIABLE_COLOR_STROKE_WIDTH = 2.5 // marker radius const DEFAULT_MARKER_RADIUS = 1.8 const VARIABLE_COLOR_MARKER_RADIUS = 2.2 +const DISCONNECTED_DOTS_MARKER_RADIUS = 2.6 // line outline const DEFAULT_LINE_OUTLINE_WIDTH = 0.5 const VARIABLE_COLOR_LINE_OUTLINE_WIDTH = 1.0 @@ -151,10 +152,14 @@ class Lines extends React.Component { return this.props.lineStrokeWidth ?? DEFAULT_STROKE_WIDTH } - @computed private get lineOutlineWidth(): number { + @computed private get outlineWidth(): number { return this.props.lineOutlineWidth ?? DEFAULT_LINE_OUTLINE_WIDTH } + @computed private get outlineColor(): string { + return this.props.backgroundColor ?? GRAPHER_BACKGROUND_DEFAULT + } + // Don't display point markers if there are very many of them for performance reasons // Note that we're using circle elements instead of marker-mid because marker performance in Safari 10 is very poor for some reason @computed private get hasMarkers(): boolean { @@ -167,14 +172,29 @@ class Lines extends React.Component { return totalPoints < 500 } + @computed private get hasDisconnectedSeries(): boolean { + return this.props.series.some((series) => !series.isConnected) + } + private seriesHasMarkers(series: RenderLineChartSeries): boolean { - if (series.hover.background || series.isProjection) return false + if ( + series.hover.background || + series.isProjection || + // if the series is connected, but there is another one that isn't connected, + // don't show markers since the connected series is likely a smoothed version + (this.hasDisconnectedSeries && series.isConnected) + ) + return false return !series.focus.background || series.hover.active } - private renderLine(series: RenderLineChartSeries): React.ReactElement { + private renderLine( + series: RenderLineChartSeries + ): React.ReactElement | void { const { hover, focus } = series + if (!series.isConnected) return + const seriesColor = series.placedPoints[0]?.color ?? DEFAULT_LINE_COLOR const color = !focus.background || hover.active @@ -190,9 +210,8 @@ class Lines extends React.Component { hover.background && !focus.background ? GRAPHER_OPACITY_MUTE : 1 const showOutline = !focus.background || hover.active - const outlineColor = - this.props.backgroundColor ?? GRAPHER_BACKGROUND_DEFAULT - const outlineWidth = strokeWidth + this.lineOutlineWidth * 2 + const outlineColor = this.outlineColor + const outlineWidth = strokeWidth + this.outlineWidth * 2 const outline = ( { const { horizontalAxis } = this.props.dualAxis const { hover, focus } = series - // If the series only contains one point, then we will always want to - // show a marker/circle because we can't draw a line. - const forceMarkers = series.placedPoints.length === 1 + const forceMarkers = + // If the series only contains one point, then we will always want to + // show a marker/circle because we can't draw a line. + series.placedPoints.length === 1 || + // If no line is plotted, we'll always want to show markers + !series.isConnected // check if we should hide markers on the chart and series level const hideMarkers = !this.hasMarkers || !this.seriesHasMarkers(series) @@ -250,6 +272,9 @@ class Lines extends React.Component { const opacity = hover.background && !focus.background ? GRAPHER_OPACITY_MUTE : 1 + const outlineColor = !series.isConnected ? this.outlineColor : undefined + const outlineWidth = !series.isConnected ? this.outlineWidth : undefined + return ( {series.placedPoints.map((value, index) => { @@ -268,6 +293,8 @@ class Lines extends React.Component { cy={value.y} r={this.markerRadius} fill={color} + stroke={outlineColor} + strokeWidth={outlineWidth} opacity={opacity} /> ) @@ -521,9 +548,9 @@ export class LineChart } @computed private get markerRadius(): number { - return this.hasColorScale - ? VARIABLE_COLOR_MARKER_RADIUS - : DEFAULT_MARKER_RADIUS + if (this.hasDisconnectedSeries) return DISCONNECTED_DOTS_MARKER_RADIUS + if (this.hasColorScale) return VARIABLE_COLOR_MARKER_RADIUS + return DEFAULT_MARKER_RADIUS } @computed get selectionArray(): SelectionArray { @@ -845,6 +872,10 @@ export class LineChart return this.hasColorScale ? 700 : 400 } + @computed get hidePoints(): boolean { + return !!this.manager.hidePoints || !!this.manager.isStaticAndSmall + } + @computed get lineLegendX(): number { return this.bounds.right - this.lineLegendWidth } @@ -975,7 +1006,7 @@ export class LineChart dualAxis={this.dualAxis} series={this.renderSeries} multiColor={this.hasColorScale} - hidePoints={manager.hidePoints || manager.isStaticAndSmall} + hidePoints={this.hidePoints} lineStrokeWidth={this.lineStrokeWidth} lineOutlineWidth={this.lineOutlineWidth} backgroundColor={this.manager.backgroundColor} @@ -1285,6 +1316,7 @@ export class LineChart points, seriesName, isProjection: column.isProjection, + isConnected: column.display?.isConnected ?? true, color: seriesColor, } } @@ -1298,6 +1330,10 @@ export class LineChart ) } + @computed private get hasDisconnectedSeries(): boolean { + return this.series.some((series) => !series.isConnected) + } + // TODO: remove, seems unused @computed get allPoints(): LinePoint[] { return this.series.flatMap((series) => series.points) @@ -1341,7 +1377,7 @@ export class LineChart } @computed get renderSeries(): RenderLineChartSeries[] { - const series: RenderLineChartSeries[] = this.placedSeries.map( + let series: RenderLineChartSeries[] = this.placedSeries.map( (series) => { return { ...series, @@ -1351,10 +1387,13 @@ export class LineChart } ) + // draw connected series on top of disconnected ones + series = sortBy(series, (series) => series.isConnected) + // sort by interaction state so that foreground series // are drawn on top of background series if (this.isHoverModeActive || this.isFocusModeActive) { - return sortBy(series, byHoverThenFocusState) + series = sortBy(series, byHoverThenFocusState) } return series diff --git a/packages/@ourworldindata/grapher/src/lineCharts/LineChartConstants.ts b/packages/@ourworldindata/grapher/src/lineCharts/LineChartConstants.ts index 8ee26f4622a..0da51b68849 100644 --- a/packages/@ourworldindata/grapher/src/lineCharts/LineChartConstants.ts +++ b/packages/@ourworldindata/grapher/src/lineCharts/LineChartConstants.ts @@ -23,6 +23,7 @@ export interface PlacedPoint { export interface LineChartSeries extends ChartSeries { isProjection?: boolean + isConnected?: boolean points: LinePoint[] } diff --git a/packages/@ourworldindata/grapher/src/schema/grapher-schema.006.yaml b/packages/@ourworldindata/grapher/src/schema/grapher-schema.006.yaml index 096173ab3df..4d47e18d50e 100644 --- a/packages/@ourworldindata/grapher/src/schema/grapher-schema.006.yaml +++ b/packages/@ourworldindata/grapher/src/schema/grapher-schema.006.yaml @@ -266,6 +266,11 @@ properties: description: | Indicates if this time series is a forward projection (if yes then this is rendered differently in e.g. line charts) + isConnected: + type: boolean + default: false + description: | + Indicates if this time series should be connected with a line. Only relevant for line charts. name: type: string description: The display string for this variable diff --git a/packages/@ourworldindata/types/src/OwidVariableDisplayConfigInterface.ts b/packages/@ourworldindata/types/src/OwidVariableDisplayConfigInterface.ts index 148d66b02fa..f44feaf8f93 100644 --- a/packages/@ourworldindata/types/src/OwidVariableDisplayConfigInterface.ts +++ b/packages/@ourworldindata/types/src/OwidVariableDisplayConfigInterface.ts @@ -21,6 +21,7 @@ export interface OwidVariableDisplayConfigInterface { includeInTable?: boolean tableDisplay?: OwidVariableDataTableConfigInterface color?: string + isConnected?: boolean } // todo: flatten onto the above diff --git a/packages/@ourworldindata/utils/src/OwidVariable.ts b/packages/@ourworldindata/utils/src/OwidVariable.ts index 5ab7b5b0cb4..74201954b69 100644 --- a/packages/@ourworldindata/utils/src/OwidVariable.ts +++ b/packages/@ourworldindata/utils/src/OwidVariable.ts @@ -29,6 +29,7 @@ class OwidVariableDisplayConfigDefaults { @observable includeInTable? = true @observable tableDisplay?: OwidVariableDataTableConfigInterface @observable color?: string = undefined + @observable isConnected?: boolean = undefined } export class OwidVariableDisplayConfig From 36139d6d97c6146d3060f394d279658230e5af5b Mon Sep 17 00:00:00 2001 From: Sophia Mersmann Date: Mon, 13 Jan 2025 09:50:01 +0100 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=94=A8=20rename=20isConnected=20to=20?= =?UTF-8?q?plotMarkersOnlyInLineChart=20update?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- adminSiteClient/DimensionCard.tsx | 13 ++++--- .../grapher/src/lineCharts/LineChart.tsx | 34 +++++++++++-------- .../src/lineCharts/LineChartConstants.ts | 2 +- .../src/schema/grapher-schema.006.yaml | 4 +-- .../src/OwidVariableDisplayConfigInterface.ts | 2 +- .../@ourworldindata/utils/src/OwidVariable.ts | 2 +- 6 files changed, 32 insertions(+), 25 deletions(-) diff --git a/adminSiteClient/DimensionCard.tsx b/adminSiteClient/DimensionCard.tsx index d812d2ba16e..87f69dbcdbe 100644 --- a/adminSiteClient/DimensionCard.tsx +++ b/adminSiteClient/DimensionCard.tsx @@ -51,8 +51,8 @@ export class DimensionCard< this.onChange() } - @action.bound onIsConnected(value: boolean) { - this.props.dimension.display.isConnected = value + @action.bound onPlotMarkersOnly(value: boolean) { + this.props.dimension.display.plotMarkersOnlyInLineChart = value this.onChange() } @@ -259,9 +259,12 @@ export class DimensionCard< )} {grapher.isLineChart && ( )}
diff --git a/packages/@ourworldindata/grapher/src/lineCharts/LineChart.tsx b/packages/@ourworldindata/grapher/src/lineCharts/LineChart.tsx index 5033d637d3d..8a879229215 100644 --- a/packages/@ourworldindata/grapher/src/lineCharts/LineChart.tsx +++ b/packages/@ourworldindata/grapher/src/lineCharts/LineChart.tsx @@ -172,17 +172,17 @@ class Lines extends React.Component { return totalPoints < 500 } - @computed private get hasDisconnectedSeries(): boolean { - return this.props.series.some((series) => !series.isConnected) + @computed private get hasMarkersOnlySeries(): boolean { + return this.props.series.some((series) => series.plotMarkersOnly) } private seriesHasMarkers(series: RenderLineChartSeries): boolean { if ( series.hover.background || series.isProjection || - // if the series is connected, but there is another one that isn't connected, - // don't show markers since the connected series is likely a smoothed version - (this.hasDisconnectedSeries && series.isConnected) + // if the series has a line, but there is another one that hasn't, then + // don't show markers since the plotted line is likely a smoothed version + (this.hasMarkersOnlySeries && !series.plotMarkersOnly) ) return false return !series.focus.background || series.hover.active @@ -193,7 +193,7 @@ class Lines extends React.Component { ): React.ReactElement | void { const { hover, focus } = series - if (!series.isConnected) return + if (series.plotMarkersOnly) return const seriesColor = series.placedPoints[0]?.color ?? DEFAULT_LINE_COLOR const color = @@ -262,7 +262,7 @@ class Lines extends React.Component { // show a marker/circle because we can't draw a line. series.placedPoints.length === 1 || // If no line is plotted, we'll always want to show markers - !series.isConnected + series.plotMarkersOnly // check if we should hide markers on the chart and series level const hideMarkers = !this.hasMarkers || !this.seriesHasMarkers(series) @@ -272,8 +272,12 @@ class Lines extends React.Component { const opacity = hover.background && !focus.background ? GRAPHER_OPACITY_MUTE : 1 - const outlineColor = !series.isConnected ? this.outlineColor : undefined - const outlineWidth = !series.isConnected ? this.outlineWidth : undefined + const outlineColor = series.plotMarkersOnly + ? this.outlineColor + : undefined + const outlineWidth = series.plotMarkersOnly + ? this.outlineWidth + : undefined return ( @@ -548,7 +552,7 @@ export class LineChart } @computed private get markerRadius(): number { - if (this.hasDisconnectedSeries) return DISCONNECTED_DOTS_MARKER_RADIUS + if (this.hasMarkersOnlySeries) return DISCONNECTED_DOTS_MARKER_RADIUS if (this.hasColorScale) return VARIABLE_COLOR_MARKER_RADIUS return DEFAULT_MARKER_RADIUS } @@ -1316,7 +1320,7 @@ export class LineChart points, seriesName, isProjection: column.isProjection, - isConnected: column.display?.isConnected ?? true, + plotMarkersOnly: column.display?.plotMarkersOnlyInLineChart, color: seriesColor, } } @@ -1330,8 +1334,8 @@ export class LineChart ) } - @computed private get hasDisconnectedSeries(): boolean { - return this.series.some((series) => !series.isConnected) + @computed private get hasMarkersOnlySeries(): boolean { + return this.series.some((series) => series.plotMarkersOnly) } // TODO: remove, seems unused @@ -1387,8 +1391,8 @@ export class LineChart } ) - // draw connected series on top of disconnected ones - series = sortBy(series, (series) => series.isConnected) + // draw lines on top of markers-only series + series = sortBy(series, (series) => !series.plotMarkersOnly) // sort by interaction state so that foreground series // are drawn on top of background series diff --git a/packages/@ourworldindata/grapher/src/lineCharts/LineChartConstants.ts b/packages/@ourworldindata/grapher/src/lineCharts/LineChartConstants.ts index 0da51b68849..749da005d8a 100644 --- a/packages/@ourworldindata/grapher/src/lineCharts/LineChartConstants.ts +++ b/packages/@ourworldindata/grapher/src/lineCharts/LineChartConstants.ts @@ -23,7 +23,7 @@ export interface PlacedPoint { export interface LineChartSeries extends ChartSeries { isProjection?: boolean - isConnected?: boolean + plotMarkersOnly?: boolean points: LinePoint[] } diff --git a/packages/@ourworldindata/grapher/src/schema/grapher-schema.006.yaml b/packages/@ourworldindata/grapher/src/schema/grapher-schema.006.yaml index 4d47e18d50e..18cc1247526 100644 --- a/packages/@ourworldindata/grapher/src/schema/grapher-schema.006.yaml +++ b/packages/@ourworldindata/grapher/src/schema/grapher-schema.006.yaml @@ -266,11 +266,11 @@ properties: description: | Indicates if this time series is a forward projection (if yes then this is rendered differently in e.g. line charts) - isConnected: + plotMarkersOnlyInLineChart: type: boolean default: false description: | - Indicates if this time series should be connected with a line. Only relevant for line charts. + Indicates if data points should be connected with a line in a line chart name: type: string description: The display string for this variable diff --git a/packages/@ourworldindata/types/src/OwidVariableDisplayConfigInterface.ts b/packages/@ourworldindata/types/src/OwidVariableDisplayConfigInterface.ts index f44feaf8f93..6c7c0a182cd 100644 --- a/packages/@ourworldindata/types/src/OwidVariableDisplayConfigInterface.ts +++ b/packages/@ourworldindata/types/src/OwidVariableDisplayConfigInterface.ts @@ -21,7 +21,7 @@ export interface OwidVariableDisplayConfigInterface { includeInTable?: boolean tableDisplay?: OwidVariableDataTableConfigInterface color?: string - isConnected?: boolean + plotMarkersOnlyInLineChart?: boolean } // todo: flatten onto the above diff --git a/packages/@ourworldindata/utils/src/OwidVariable.ts b/packages/@ourworldindata/utils/src/OwidVariable.ts index 74201954b69..7ffbb8e894a 100644 --- a/packages/@ourworldindata/utils/src/OwidVariable.ts +++ b/packages/@ourworldindata/utils/src/OwidVariable.ts @@ -29,7 +29,7 @@ class OwidVariableDisplayConfigDefaults { @observable includeInTable? = true @observable tableDisplay?: OwidVariableDataTableConfigInterface @observable color?: string = undefined - @observable isConnected?: boolean = undefined + @observable plotMarkersOnlyInLineChart?: boolean = undefined } export class OwidVariableDisplayConfig