-
-
Notifications
You must be signed in to change notification settings - Fork 9k
Add legends to XY charts #7724
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Add legends to XY charts #7724
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| 'mermaid': minor | ||
| --- | ||
|
|
||
| feat: add legends for named XY chart line and bar series | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: I think making But, just in case, can you add a short line here that explains how to disable it for users that do want to disable it? |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,26 @@ describe('XY Chart', () => { | |
| {} | ||
| ); | ||
| }); | ||
| it('should render legends for named plots', () => { | ||
| renderGraph( | ||
| ` | ||
| xychart-beta | ||
| title "An Example Chart" | ||
| x-axis ["90d", "60d", "30d", "7d", "1d", "Current"] | ||
| y-axis "Seconds" 0 --> 198.2 | ||
| line "avg" [48.1, 41.5, 45.7, 72.8, 67.7, 59.9] | ||
| line "p50" [38.2, 36.8, 39.7, 54.5, 49.0, 38.4] | ||
| line "p95" [112.2, 75.3, 103.0, 177.0, 180.2, 109.4] | ||
|
Comment on lines
+102
to
+104
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue(blocking): We don't have any sceenshot tests for legends with bars instead of lines. suggestion: Can you change one of these to use a bars instead of line? |
||
| `, | ||
| { screenshot: false } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue(blocking): Currently, we don't have any visual regression tests of your new feature. Can we remove this line? If we take a screenshot of it, then @argos-ci should warn us if we accidentally change how legends look, in the future. |
||
| ); | ||
|
|
||
| cy.get('g.legend text').should('have.length', 3); | ||
| cy.get('g.legend').should('contain.text', 'avg'); | ||
| cy.get('g.legend').should('contain.text', 'p50'); | ||
| cy.get('g.legend').should('contain.text', 'p95'); | ||
| cy.get('g.legend path').should('have.length', 3); | ||
| }); | ||
| it('Decimals and negative numbers are supported', () => { | ||
| imgSnapshotTest( | ||
| ` | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,213 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { ChartLegend } from './legend.js'; | ||
| import type { XYChartConfig, XYChartData, XYChartThemeConfig } from '../interfaces.js'; | ||
| import type { TextDimensionCalculator } from '../textDimensionCalculator.js'; | ||
|
|
||
| const textDimensionCalculator: TextDimensionCalculator = { | ||
| getMaxDimension: (texts, fontSize) => ({ | ||
| width: Math.max(...texts.map((text) => text.length)) * fontSize, | ||
| height: fontSize, | ||
| }), | ||
| }; | ||
|
|
||
| const chartConfig: XYChartConfig = { | ||
| width: 700, | ||
| height: 500, | ||
| titleFontSize: 20, | ||
| titlePadding: 10, | ||
| showTitle: true, | ||
| showLegend: true, | ||
| legendFontSize: 14, | ||
| legendPadding: 10, | ||
| showDataLabel: false, | ||
| showDataLabelOutsideBar: false, | ||
| chartOrientation: 'vertical', | ||
| plotReservedSpacePercent: 50, | ||
| xAxis: { | ||
| showLabel: true, | ||
| labelFontSize: 14, | ||
| labelPadding: 5, | ||
| showTitle: true, | ||
| titleFontSize: 16, | ||
| titlePadding: 5, | ||
| showTick: true, | ||
| tickLength: 5, | ||
| tickWidth: 2, | ||
| showAxisLine: true, | ||
| axisLineWidth: 2, | ||
| }, | ||
| yAxis: { | ||
| showLabel: true, | ||
| labelFontSize: 14, | ||
| labelPadding: 5, | ||
| showTitle: true, | ||
| titleFontSize: 16, | ||
| titlePadding: 5, | ||
| showTick: true, | ||
| tickLength: 5, | ||
| tickWidth: 2, | ||
| showAxisLine: true, | ||
| axisLineWidth: 2, | ||
| }, | ||
| }; | ||
|
|
||
| const chartThemeConfig: XYChartThemeConfig = { | ||
| backgroundColor: '#fff', | ||
| titleColor: '#111', | ||
| dataLabelColor: '#222', | ||
| legendTextColor: '#333', | ||
| xAxisLabelColor: '#444', | ||
| xAxisTitleColor: '#555', | ||
| xAxisTickColor: '#666', | ||
| xAxisLineColor: '#777', | ||
| yAxisLabelColor: '#888', | ||
| yAxisTitleColor: '#999', | ||
| yAxisTickColor: '#aaa', | ||
| yAxisLineColor: '#bbb', | ||
| plotColorPalette: '#f00,#0f0', | ||
| }; | ||
|
|
||
| const chartData: XYChartData = { | ||
| title: 'Latency', | ||
| xAxis: { | ||
| type: 'band', | ||
| title: '', | ||
| categories: ['90d', '60d'], | ||
| }, | ||
| yAxis: { | ||
| type: 'linear', | ||
| title: 'Seconds', | ||
| min: 0, | ||
| max: 100, | ||
| }, | ||
| plots: [ | ||
| { | ||
| type: 'line', | ||
| title: 'avg', | ||
| strokeFill: '#f00', | ||
| strokeWidth: 2, | ||
| data: [ | ||
| ['90d', 40], | ||
| ['60d', 50], | ||
| ], | ||
| }, | ||
| { | ||
| type: 'bar', | ||
| title: 'p95', | ||
| fill: '#0f0', | ||
| data: [ | ||
| ['90d', 80], | ||
| ['60d', 90], | ||
| ], | ||
| }, | ||
| { | ||
| type: 'line', | ||
| title: '', | ||
| strokeFill: '#00f', | ||
| strokeWidth: 2, | ||
| data: [ | ||
| ['90d', 30], | ||
| ['60d', 35], | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: This is quite a lot of hard-coded data, which means that future changes will probably need to modify this file. suggestion: Is there any way we can just load some default data from |
||
| describe('ChartLegend', () => { | ||
| it('renders marker and label drawables for named line and bar plots', () => { | ||
| const legend = new ChartLegend( | ||
| textDimensionCalculator, | ||
| chartConfig, | ||
| chartData, | ||
| chartThemeConfig | ||
| ); | ||
|
|
||
| expect(legend.calculateSpace({ width: 200, height: 200 })).toEqual({ | ||
| width: 77.4, | ||
| height: 55, | ||
| }); | ||
|
|
||
| legend.setBoundingBoxXY({ x: 100, y: 50 }); | ||
| const drawables = legend.getDrawableElements(); | ||
|
|
||
| expect(drawables).toHaveLength(3); | ||
| expect(drawables[0]).toMatchObject({ | ||
| groupTexts: ['legend', 'markers'], | ||
| type: 'rect', | ||
| data: [ | ||
| { | ||
| x: 110, | ||
| y: 81, | ||
| width: 10.5, | ||
| height: 10.5, | ||
| fill: '#0f0', | ||
| strokeFill: '#0f0', | ||
| strokeWidth: 0, | ||
| }, | ||
| ], | ||
| }); | ||
| expect(drawables[1]).toMatchObject({ | ||
| groupTexts: ['legend', 'markers'], | ||
| type: 'path', | ||
| data: [ | ||
| { | ||
| path: 'M 110,65.25 L 120.5,65.25', | ||
| strokeFill: '#f00', | ||
| strokeWidth: 2, | ||
| }, | ||
| ], | ||
| }); | ||
| expect(drawables[2]).toMatchObject({ | ||
| groupTexts: ['legend', 'label'], | ||
| type: 'text', | ||
| data: [ | ||
| { | ||
| text: 'avg', | ||
| x: 125.4, | ||
| y: 65.25, | ||
| fill: '#333', | ||
| fontSize: 14, | ||
| rotation: 0, | ||
| verticalPos: 'middle', | ||
| horizontalPos: 'left', | ||
| }, | ||
| { | ||
| text: 'p95', | ||
| x: 125.4, | ||
| y: 86.25, | ||
| fill: '#333', | ||
| fontSize: 14, | ||
| rotation: 0, | ||
| verticalPos: 'middle', | ||
| horizontalPos: 'left', | ||
| }, | ||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| it('does not render when legends are disabled or no named plots fit', () => { | ||
| const disabledLegend = new ChartLegend( | ||
| textDimensionCalculator, | ||
| { ...chartConfig, showLegend: false }, | ||
| chartData, | ||
| chartThemeConfig | ||
| ); | ||
| expect(disabledLegend.calculateSpace({ width: 200, height: 200 })).toEqual({ | ||
| width: 0, | ||
| height: 0, | ||
| }); | ||
| expect(disabledLegend.getDrawableElements()).toEqual([]); | ||
|
|
||
| const crampedLegend = new ChartLegend( | ||
| textDimensionCalculator, | ||
| chartConfig, | ||
| chartData, | ||
| chartThemeConfig | ||
| ); | ||
| expect(crampedLegend.calculateSpace({ width: 20, height: 20 })).toEqual({ | ||
| width: 0, | ||
| height: 0, | ||
| }); | ||
| expect(crampedLegend.getDrawableElements()).toEqual([]); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: This makes it just a little bit easier to parse the
CHANGELOG.mdfile and release notes for changes, if you only care about some diagram type.