Skip to content
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

Jump to bottom

Support dynamically sized legend elements #9532

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+15 −11
Draft

Support dynamically sized legend elements #9532

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/helpers/helpers.canvas.js
View file
Open in desktop
Expand Up @@ -135,7 +135,7 @@ export function drawPoint(ctx, options, x, y) {
ctx.save();
ctx.translate(x, y);
ctx.rotate(rad);
ctx.drawImage(style, -style.width / 2, -style.height / 2, style.width, style.height);
ctx.drawImage(style, 0, -style.height / 2, style.width, style.height);
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this change? When this function is normally called, x,y is the center of the point and thus the center of the image

Copy link
Author

Choose a reason for hiding this comment

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

Centering the image looks to be done because there is an assumed (fixed) width where the image will exist.

With this approach, because the space for the legend image is identical to the size of the legend image itself, there's no need to center it.

Screenshot from 2021-08-11 13-39-22

That being said this was me hacking around and it resulted in no padding so centering might actually be a good idea if the "boxWidth" (legend icon width) is expected to include padding.

ctx.restore();
return;
}
Expand Down
24 changes: 14 additions & 10 deletions src/plugins/plugin.legend.js
View file
Open in desktop
Expand Up @@ -170,7 +170,9 @@ export class Legend extends Element {
let row = -1;
let top = -lineHeight;
me.legendItems.forEach((legendItem, i) => {
const itemWidth = boxWidth + (fontSize / 2) + ctx.measureText(legendItem.text).width;
const pointStyle = legendItem?.pointStyle;
Copy link
Member

Choose a reason for hiding this comment

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

We are at es2018, so no optional chaining. Althought it looks like all the supported browsers already support optional chaining, we should wait for a major version before changing the env requirements.

const pointWidth = pointStyle?.offsetWidth || pointStyle?.width || boxWidth;
Copy link
Author

Choose a reason for hiding this comment

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

Would make sense to render this to the DOM in a visibility: hidden element, get offsetWidth, then remove from dom

Copy link
Member

Choose a reason for hiding this comment

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

I think we should avoid DOM manipulation.

const itemWidth = pointWidth + fontSize / 2 + ctx.measureText(legendItem.text).width;

if (i === 0 || lineWidths[lineWidths.length - 1] + itemWidth + 2 * padding > maxWidth) {
totalHeight += lineHeight;
Expand Down Expand Up @@ -202,7 +204,8 @@ export class Legend extends Element {
let col = 0;

me.legendItems.forEach((legendItem, i) => {
const itemWidth = boxWidth + (fontSize / 2) + ctx.measureText(legendItem.text).width;
const pointWidth = legendItem?.pointStyle?.offsetWidth || boxWidth;
const itemWidth = pointWidth + fontSize / 2 + ctx.measureText(legendItem.text).width;

// If too tall, go to new column
if (i > 0 && currentColHeight + itemHeight + 2 * padding > heightLimit) {
Expand Down Expand Up @@ -327,7 +330,7 @@ export class Legend extends Element {
// Recalculate x and y for drawPoint() because its expecting
// x and y to be center of figure (instead of top left)
const drawOptions = {
radius: boxWidth * Math.SQRT2 / 2,
radius: (legendItem.boxWidth) || boxWidth * Math.SQRT2 / 2,
pointStyle: legendItem.pointStyle,
rotation: legendItem.rotation,
borderWidth: lineWidth
Expand All @@ -340,8 +343,8 @@ export class Legend extends Element {
} else {
// Draw box as legend symbol
// Adjust position when boxHeight < fontSize (want it centered)
const yBoxTop = y + Math.max((fontSize - boxHeight) / 2, 0);
const xBoxLeft = rtlHelper.leftForLtr(x, boxWidth);
const yBoxTop = y + Math.max((fontSize - (legendItem.boxHeight || boxHeight)) / 2, 0);
const xBoxLeft = rtlHelper.leftForLtr(x, legendItem.boxWidth || boxWidth);
const borderRadius = toTRBLCorners(legendItem.borderRadius);

ctx.beginPath();
Expand All @@ -350,9 +353,9 @@ export class Legend extends Element {
addRoundedRectPath(ctx, {
x: xBoxLeft,
y: yBoxTop,
w: boxWidth,
h: boxHeight,
radius: borderRadius,
w: legendItem.boxWidth || boxWidth,
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this change?

Copy link
Author

Choose a reason for hiding this comment

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

I found it unusual that boxWidth could be set for all legend items, but not an individual one.

Might just be me - can revert if needed 👍

h: legendItem.boxHeight || boxHeight,
radius: borderRadius
});
} else {
ctx.rect(xBoxLeft, yBoxTop, boxWidth, boxHeight);
Expand Down Expand Up @@ -401,7 +404,8 @@ export class Legend extends Element {

const textWidth = ctx.measureText(legendItem.text).width;
const textAlign = rtlHelper.textAlign(legendItem.textAlign || (legendItem.textAlign = labelOpts.textAlign));
const width = boxWidth + halfFontSize + textWidth;
const pointerWidth = legendItem.pointStyle?.offsetWidth || legendItem.pointStyle?.width || boxWidth;
const width = pointerWidth + halfFontSize + textWidth;
let x = cursor.x;
let y = cursor.y;

Expand All @@ -423,7 +427,7 @@ export class Legend extends Element {

drawLegendBox(realX, y, legendItem);

x = _textX(textAlign, x + boxWidth + halfFontSize, isHorizontal ? x + width : me.right, opts.rtl);
x = _textX(textAlign, x + pointerWidth + halfFontSize, isHorizontal ? x + width : me.right, opts.rtl);

// Fill the actual label
fillText(rtlHelper.x(x), y, legendItem);
Expand Down