Add legends to XY charts#7724
Conversation
Preserve named line and bar series as legend entries so multi-series XY charts can identify their plotted data. Co-authored-by: Cursor <cursoragent@cursor.com>
Follow Mermaid contribution guidance by marking the new docs section with the release token and adding a changeset. Co-authored-by: Cursor <cursoragent@cursor.com>
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🦋 Changeset detectedLatest commit: 3934d7c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #7724 +/- ##
==========================================
- Coverage 3.30% 3.29% -0.01%
==========================================
Files 561 561
Lines 58355 58495 +140
Branches 873 875 +2
==========================================
+ Hits 1928 1930 +2
- Misses 56427 56565 +138
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
Add focused unit coverage for legend rendering, builder layout integration, plot title persistence, and theme defaults. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Hi @xdumaine. Thanks for this PR. [sisyphus-bot] What's working well 🎉 Comprehensive test coverage for a first-time contributor. Four new test files covering legend unit tests, chart builder integration, DB title persistence, and all 11 themes. 79/79 unit tests + 🎉 Clean architectural fit. ChartLegend implements the existing ChartComponent interface without any deviation. Reserving space before expanding the plot to fill remaining width is exactly the right 🎉 Elegant backwards-compatible syntax. Reusing line "name" [...] (already parseable; title was previously ignored) means zero new grammar and existing unnamed charts are completely unaffected. 🎉 Correct sanitization. title: textSanitizer(title.text) in both setLineData and setBarData matches the established pattern for every other user-supplied text field in this diagram. No XSS path — 🎉 Config schema first. Schema additions drive config.type.ts regeneration; PR body includes pnpm types:build-config in the test plan. The regenerated file matches the schema additions exactly. Things to address 🟡 [important] showLegend: true by default changes rendering for existing documents config.schema.yaml: Any existing diagram that already uses line "name" [...] or bar "name" [...] syntax — including documents on GitHub, GitLab, Obsidian — will now automatically render a legend box, changing their CLAUDE.md is explicit: "Mermaid is rendered server-side by GitHub/GitLab — rendering changes affect millions of existing documents. Treat visual output changes as potentially breaking." The counter-argument (acknowledged in the PR description) is that users who wrote line "avg" [...] expected the name to mean something — they were already opting in to the feature by using the I would like some additional input from other maintainers if we should go for showLegend: true as default or flip to false. @aloisklink , @knsv ? The existing Cypress imgSnapshotTest cases don't use named series, so no baseline failures are expected regardless. 🟢 [nit] Cypress legend test uses screenshot: false only cypress/integration/rendering/xychart/xyChart.spec.js: The test correctly verifies DOM structure (element counts and text content). But without an imgSnapshotTest call, visual regressions — wrong marker size, misaligned text, wrong color — won't be Security No XSS or injection issues. Full trace:
getPlotColorFromPalette() supplies fill/strokeFill values set as D3 .attr('fill', ...) / .attr('stroke', ...) — the existing risk surface, no new exposure. Other checks
|
|
Great call out re: defaulting the legend to true. However, based on the end of that block (quoted below) I'm going to wait for feedback before flipping that. I'm happy to go either way and glad to make that change once there's consensus. Thank you for the review!
|
|
@aloisklink @knsv any preference? I'm glad to update it per your guidance. Would love to get this landed. |
aloisklink
left a comment
aloisklink
left a comment
There was a problem hiding this comment.
Great work on this, @xdumaine! I'm really like this!
I think keeping showLegend: true by default is fine. Most people would probably be happy to see this, and it's not going to significantly negatively affect this XY charts for other users.
But on the off chance someone doesn't like it, could you maybe add a short instruction on how to disable this in the changeset?
I've done a quick review of your code as well, and I think the main blocking issue I spotted was the lack of visual regression E2E tests (the current @argos-ci changes are all flakiness that should be gone the next time you push), but please see my comments for more information!
I'm going to ping @subhash-halder, the original author of XY charts, in case they want to review this (and also so they can see how awesome this feature is!).
| 'mermaid': minor | ||
| --- | ||
|
|
||
| feat: add legends for named XY chart line and bar series |
There was a problem hiding this comment.
nitpick: This makes it just a little bit easier to parse the CHANGELOG.md file and release notes for changes, if you only care about some diagram type.
| feat: add legends for named XY chart line and bar series | |
| feat(xyChart): add legends for named line and bar series |
| 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] | ||
| `, | ||
| { screenshot: false } |
There was a problem hiding this comment.
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.
| 'mermaid': minor | ||
| --- | ||
|
|
||
| feat: add legends for named XY chart line and bar series |
There was a problem hiding this comment.
suggestion: I think making showLegend default to true is okay for this feature. Most people would probably be happy to see this, and it's not going to significantly negatively affect this XY charts for other users.
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?
| const fontSize = this.chartConfig.legendFontSize; | ||
| const markerSize = fontSize * LEGEND_MARKER_TO_FONT_RATIO; | ||
| const markerSpacing = fontSize * LEGEND_MARKER_SPACING_TO_FONT_RATIO; | ||
| const itemSpacing = fontSize * LEGEND_ITEM_SPACING_TO_FONT_RATIO; |
There was a problem hiding this comment.
issue(non-blocking): Not a blocking issue, but this is duplicated above in https://github.com/xdumaine/mermaid/blob/3934d7c49034146f6b1969ee6628e236b135ac1e/packages/mermaid/src/diagrams/xychart/chartBuilder/components/legend.ts#L47-L50
If there's any way you can reduce these, without making the code more complicated, please do!
| }, | ||
| ]; | ||
| }), | ||
| }, |
There was a problem hiding this comment.
suggestion:
Would it be better to instead combine the code, e.g. doing something like:
...this.visiblePlots.map((plot, index) => {
if (isBarPlot(plot)) {
return {
groupTexts: ['legend', 'markers'],
type: 'rect',
// ....
}
} else {
return {
groupTexts: ['legend', 'markers'],
type: 'path',
// ...
};
}
}),It means we can avoid the .flatMap and I think it's makes this easier to understand!
| }, | ||
| ], | ||
| }; | ||
|
|
There was a problem hiding this comment.
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 getConfig()/getChartThemeConfig()?
| 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: 'An Example Chart', | ||
| xAxis: { | ||
| type: 'band', | ||
| title: '', | ||
| categories: ['90d', '60d', '30d'], | ||
| }, | ||
| yAxis: { | ||
| type: 'linear', | ||
| title: 'Seconds', | ||
| min: 0, | ||
| max: 200, | ||
| }, | ||
| plots: [ | ||
| { | ||
| type: 'line', | ||
| title: 'avg', | ||
| strokeFill: '#f00', | ||
| strokeWidth: 2, | ||
| data: [ | ||
| ['90d', 48.1], | ||
| ['60d', 41.5], | ||
| ['30d', 45.7], | ||
| ], | ||
| }, | ||
| { | ||
| type: 'bar', | ||
| title: 'p95', | ||
| fill: '#0f0', | ||
| data: [ | ||
| ['90d', 112.2], | ||
| ['60d', 75.3], | ||
| ['30d', 103.0], | ||
| ], | ||
| }, | ||
| ], | ||
| }; |
There was a problem hiding this comment.
issue: Again, lots of hard-coded data here.
suggestion: Do we really need this test file, especially since we have the unit tests for legend.ts and the E2E tests?
It might just be easier to delete this file, if it's not offering any value.
| title: string; | ||
| strokeFill: string; | ||
| strokeWidth: number; | ||
| data: SimplePlotDataType; | ||
| } | ||
|
|
||
| export interface BarPlotData { | ||
| type: 'bar'; | ||
| title: string; |
There was a problem hiding this comment.
suggestion: Is there any way we can document that empty titles are treated differently?
Maybe something like:
export interface BarPlotData {
type: 'bar';
/**
* The title of this plot, or `""` if there is no title.
*/
title: string;Converting this to be title: string | undefined; might be better, but it would require code changes, so I don't think you need to go through the effort of that unless you really want to.
| import { getThemeVariables as getBaseThemeVariables } from './theme-base.js'; | ||
| import { getThemeVariables as getDarkThemeVariables } from './theme-dark.js'; | ||
| import { getThemeVariables as getDefaultThemeVariables } from './theme-default.js'; | ||
| import { getThemeVariables as getForestThemeVariables } from './theme-forest.js'; | ||
| import { getThemeVariables as getNeoDarkThemeVariables } from './theme-neo-dark.js'; | ||
| import { getThemeVariables as getNeoThemeVariables } from './theme-neo.js'; | ||
| import { getThemeVariables as getNeutralThemeVariables } from './theme-neutral.js'; | ||
| import { getThemeVariables as getReduxColorThemeVariables } from './theme-redux-color.js'; | ||
| import { getThemeVariables as getReduxDarkColorThemeVariables } from './theme-redux-dark-color.js'; | ||
| import { getThemeVariables as getReduxDarkThemeVariables } from './theme-redux-dark.js'; | ||
| import { getThemeVariables as getReduxThemeVariables } from './theme-redux.js'; | ||
|
|
||
| const themes = [ | ||
| ['base', getBaseThemeVariables], | ||
| ['dark', getDarkThemeVariables], | ||
| ['default', getDefaultThemeVariables], | ||
| ['forest', getForestThemeVariables], | ||
| ['neo-dark', getNeoDarkThemeVariables], | ||
| ['neo', getNeoThemeVariables], | ||
| ['neutral', getNeutralThemeVariables], | ||
| ['redux-color', getReduxColorThemeVariables], | ||
| ['redux-dark-color', getReduxDarkColorThemeVariables], | ||
| ['redux-dark', getReduxDarkThemeVariables], | ||
| ['redux', getReduxThemeVariables], | ||
| ]; |
There was a problem hiding this comment.
issue(blocking): Please use
mermaid/packages/mermaid/src/themes/index.js
Lines 1 to 47 in 2a51ae4
| it.each(themes)('%s allows overriding legend text color', (_name, getThemeVariables) => { | ||
| const theme = getThemeVariables({ | ||
| xyChart: { | ||
| legendTextColor: '#123456', | ||
| }, | ||
| }); | ||
|
|
||
| expect(theme.xyChart.legendTextColor).toBe('#123456'); |
There was a problem hiding this comment.
issue: This test seems to be testing the getThemeVariables instead of anything XYChart.
suggestion: Please remove! Maybe in the future we can add a separate test for this, but it doesn't need to be in this PR.
3 participants
📑 Summary
Adds legends for named XY chart line and bar series. Authors can already write labels like
line "avg" [...]; this change preserves those labels and renders them in a right-side legend whenshowLegendis enabled.Fixes #5292.
Motivation
XY charts support multiple line and bar plots, but readers currently have no built-in way to map each plotted color to a series name. That makes examples like average, p50, and p95 latency lines hard to interpret without surrounding text or manually faked legends.
This PR makes the existing named-series syntax useful in rendered output while keeping unnamed plots unchanged.
📏 Design Decisions
line "name" [...]andbar "name" [...]; no new diagram syntax is required.ChartLegendlayout component that reserves right-side space before the plot expands into remaining width.xyChart.showLegend,legendFontSize,legendPadding, andthemeVariables.xyChart.legendTextColorfor configuration/theming.(v<MERMAID_RELEASE_VERSION>+)marker and includes a minor changeset.Screenshots
Full chart with legend:
Legend detail:
Disclaimer / Methods
I used Cursor's AI coding agent with GPT 5.5. to help inspect Mermaid's XY chart and pie legend implementations, draft the implementation, update docs/config/types, and run verification. I reviewed the generated changes, corrected the implementation where needed, checked the contribution guidelines and PR template, and validated behavior locally with parser, TypeScript, lint, and focused Cypress rendering tests.
Test plan
pnpm --filter mermaid types:build-configpnpm exec prettier --write "packages/mermaid/src/diagrams/xychart/**/*.{ts,js}" "packages/mermaid/src/themes/*.js" "packages/mermaid/src/docs/syntax/xyChart.md" "docs/syntax/xyChart.md" "cypress/integration/rendering/xychart/xyChart.spec.js" "packages/mermaid/src/schemas/config.schema.yaml" "packages/mermaid/src/config.type.ts"pnpm exec vitest run packages/mermaid/src/diagrams/xychart/parser/xychart.jison.spec.tspnpm exec vitest run packages/mermaid/src/diagrams/xychart/chartBuilder/components/legend.spec.ts packages/mermaid/src/diagrams/xychart/chartBuilder/index.spec.ts packages/mermaid/src/diagrams/xychart/xychartDb.spec.ts packages/mermaid/src/diagrams/xychart/parser/xychart.jison.spec.ts packages/mermaid/src/themes/theme-xychart.spec.js(79/79 passing)pnpm exec vitest run --coverage packages/mermaid/src/diagrams/xychart/chartBuilder/components/legend.spec.ts packages/mermaid/src/diagrams/xychart/chartBuilder/index.spec.ts packages/mermaid/src/diagrams/xychart/xychartDb.spec.ts packages/mermaid/src/themes/theme-xychart.spec.jspnpm test:check:tscpnpm exec eslint --quiet packages/mermaid/src/diagrams/xychart/chartBuilder/components/legend.ts packages/mermaid/src/diagrams/xychart/chartBuilder/orchestrator.ts packages/mermaid/src/diagrams/xychart/chartBuilder/interfaces.ts packages/mermaid/src/diagrams/xychart/xychartDb.ts cypress/integration/rendering/xychart/xyChart.spec.jsMERMAID_PORT=9000 pnpm run load:env -- start-server-and-test dev http://localhost:9000/ "cypress run --spec cypress/integration/rendering/xychart/xyChart.spec.js"(38/38 passing)📋 Tasks
MERMAID_RELEASE_VERSIONis used for all new features.pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Made with Cursor