Skip to content

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

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.

Sign up for GitHub

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

Open
timqian wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/Pie.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,12 +128,11 @@ class Pie {
const legendItems = this.data.datasets[0].data
.map((data, i) => ({ color: this.options.dataColors[i], text: this.data.labels[i] }));

// move legend down to prevent overlaping with title
const legendG = this.svgEl.append('g')
.attr('transform', 'translate(0, 30)');

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

Comment on lines +132 to +134
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.
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.
items: legendItems,
position: this.options.legendPosition,
unxkcdify: this.options.unxkcdify,
Expand Down
6 changes: 1 addition & 5 deletions src/Radar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,11 +229,7 @@ class Radar {
const legendItems = this.data.datasets
.map((data, i) => ({ color: this.options.dataColors[i], text: data.label || '' }));

// move legend down to prevent overlaping with title
const legendG = this.svgEl.append('g')
.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.
items: legendItems,
position: this.options.legendPosition,
unxkcdify: this.options.unxkcdify,
Expand Down
Loading