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.

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

Legend Symbol Parameter [New Feature] #4811

Closed
wants to merge 15 commits into from
2 changes: 1 addition & 1 deletion docs/configuration/legend.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ Items passed to the legend `onClick` function are the ones returned from `labels
// Legend symbol to display if point style is not used.
symbol: String

// Width of legend symbol if point style is not used.
// Width of legend symbol if point style is not used.
boxWidth: Number
Copy link
Member

Choose a reason for hiding this comment

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

My only comment here is that I think it would be better to remove legendSymbolLarge and imply it if boxWidth is set. So if boxWidth !== undefined then it will be used, otherwise it will be set to fontSize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I'm not wrong, boxWidth is defaulted to 40 so it will be always defined. If we remove default value it may impact current users upgrading to that new version.

Copy link
Member

Choose a reason for hiding this comment

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

ahh, I missed that. To make that clearer I would suggest a rename to useFontSize or something. Let's wait to rename it until @simonbrunel gets a chance to review this

}
```
Expand Down
7 changes: 6 additions & 1 deletion src/helpers/helpers.canvas.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ var exports = module.exports = {
},

drawPoint: function(ctx, style, radius, x, y, rotation) {
// call draw Symbol with converted radius to width and height
// call drawSymbol with converted radius to width and height
// and move x, y to the top left corner
this.drawSymbol(ctx, style, radius * 2, radius * 2, x - radius, y - radius, rotation, true);
},
Expand Down Expand Up @@ -80,6 +80,11 @@ var exports = module.exports = {
}
var radius = width / 2;
var yRadius = height / 2;

// some symbols are not usign the full width when they're called as a point
benmccann marked this conversation as resolved.
Show resolved Hide resolved
// e.g. rect, rectRounded, rectRot, crossRot and star
// Following variables are used to define the pading value for those symbols
// the full width and height is used when symbol is called as legend.
var padLeft = isPoint ? radius - width / 2 / Math.sqrt(2) : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment explaining why padding is necessary and what isPoint is doing? I'm not sure what that part is for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added comments on that. In the production version some symbols can only be called as point and they are not usign the full width since the size is based on the radius. When is used as a legend, I want to be sure that the symbol width = the width value pass as parameter. So to not impact the current behavior (when symbol is called for a point) I did that logic for the impacted symbols. I.e. Add padding when called as a point (not usign the full width) and use the full width (no padding) when called as a legend symbol.

var padRight = isPoint ? radius + width / 2 / Math.sqrt(2) : width;
var padTop = isPoint ? radius - height / 2 / Math.sqrt(2) : 0;
Expand Down