Skip to content

fix: pass svgEl instead of g element to addLegend#104

Open
timqian wants to merge 1 commit intomasterfrom
fix/radar-legend-not-showing
Open

fix: pass svgEl instead of g element to addLegend#104
timqian wants to merge 1 commit intomasterfrom
fix/radar-legend-not-showing

Conversation

@timqian
Copy link
Copy Markdown
Owner

@timqian timqian commented Mar 23, 2026

Problem

Radar and Pie charts have legends that don't show up. The root cause is that the addLegend function expects to append SVG elements to the parent, but these charts were passing a <g> (group) element instead of the main <svg> element.

According to SVG specifications, you cannot directly append an <svg> element inside a <g> element, which is why the legend fails to render.

Solution

Pass this.svgEl (the main SVG element) to addLegend instead of a temporary <g> element.

Changes

  • Radar.js: Pass this.svgEl to addLegend instead of legendG
  • Pie.js: Pass this.svgEl to addLegend instead of legendG
  • Remove unnecessary <g> element creation and transform

Testing

Legends should now appear correctly on Radar and Pie charts when showLegend is enabled.

Fixes #101

The addLegend function expects to append SVG elements to the parent,
but Radar and Pie charts were passing a <g> element instead of the
main <svg> element. This caused legends to not render properly.

Changes:
- Radar.js: Pass this.svgEl to addLegend instead of legendG
- Pie.js: Pass this.svgEl to addLegend instead of legendG
- Remove unnecessary g element creation and transform

Fixes issue #101: Legends not showing up for charts
Copilot AI review requested due to automatic review settings March 23, 2026 09:06
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes missing legends on Radar and Pie charts by changing the element passed to addLegend, so the legend can be appended/rendered correctly within the chart SVG structure.

Changes:

  • Update Radar to call addLegend(this.svgEl, ...) instead of passing a translated legend <g>.
  • Update Pie to call addLegend(this.svgEl, ...) and remove the translated legend <g> wrapper.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/Radar.js Switch legend parent to the root SVG element for legend rendering.
src/Pie.js Switch legend parent to the root SVG element and refactor legend setup (currently introduces a regression).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

.attr('transform', 'translate(0, 30)');

addLegend(legendG, {
addLegend(this.svgEl, {
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Radar: removing the translated legend group means the legend will be positioned starting at y=0 (via addLegend), which will overlap the chart title (addLabels.title renders at y=30). Previously the legend was shifted down by 30px to avoid this; consider restoring an offset when a title is present (either by wrapping in a translated again, or by adding a configurable top padding/offset passed to addLegend).

Suggested change
addLegend(this.svgEl, {
// If a title is present, shift the legend down to avoid overlapping it
let legendParent = this.svgEl;
if (this.options.title) {
legendParent = this.svgEl.append('g')
.attr('transform', 'translate(0,30)');
}
addLegend(legendParent, {

Copilot uses AI. Check for mistakes.
Comment on lines +132 to +134
const legendItems = this.data.datasets
.map((data, i) => ({ color: this.options.dataColors[i], text: data.label || '' }));

Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pie: legendItems is computed from slice labels (datasets[0].data + this.data.labels) but then shadowed inside the showLegend block with a different legendItems derived from this.data.datasets (dataset labels). This makes the pie legend incorrect (often only 1 item) and also leaves the first legendItems unused. Use a single legendItems based on slice labels for the legend, and avoid redeclaring/shadowing it in the block.

Suggested change
const legendItems = this.data.datasets
.map((data, i) => ({ color: this.options.dataColors[i], text: data.label || '' }));

Copilot uses AI. Check for mistakes.
const legendItems = this.data.datasets
.map((data, i) => ({ color: this.options.dataColors[i], text: data.label || '' }));

addLegend(this.svgEl, {
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pie: removing the translated legend reintroduces the original title/legend overlap problem (title is rendered at y=30 and addLegend positions legends starting near y=0 for up-left/up-right). If the legend is intended to sit below the title, keep an explicit y-offset when a title is present (e.g., wrap the legend call in a translated group or extend addLegend to support a top offset).

Suggested change
addLegend(this.svgEl, {
// If there is a title, render the legend in a translated group so it
// appears below the title (which is drawn at y = 30).
let legendParent = this.svgEl;
if (this.title) {
legendParent = this.svgEl.append('g')
.attr('class', 'xkcd-legend-group')
.attr('transform', 'translate(0, 30)');
}
addLegend(legendParent, {

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Legends not showing up for charts

2 participants