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

Add option to allow square legend symbols at different sizes #4890

Closed
wants to merge 3 commits into from

Conversation

soniiic
Copy link

@soniiic soniiic commented Oct 25, 2017

If the useSquareBox option is set to true, the height is set to the same as the boxWidth option. It places it vertically central to the label. It also ensures the legend symbol never exceeds the size of the font size.

Demo: https://codepen.io/soniiic/pen/zEgrGb

If the useSquareBox option is set to true, the height is set to the same as the boxWidth option. It places it vertically central to the label. It also ensures the legend symbol never exceeds the size of the font size.
@etimberg
Copy link
Member

@soniiic thanks for PR this. I haven't looked too deeply yet. Would it be easier to simply create a boxHeight param? It's a bit more general but might cover more use cases.

@simonbrunel thoughts?

@simonbrunel
Copy link
Member

I agree, boxHeight is more flexible and in line with boxWidth, so I would tend to that solution as well. We also need to think the API to be consistent with #4496, #4638 and #4811 (and certainly other related requests)

@soniiic
Copy link
Author

soniiic commented Oct 26, 2017

Thanks for your consideration. My requirement was that it was square but I agree that adding boxHeight would allow more general usage. I have updated the code to allow the user to assign boxHeight in the label options

Demo updated: https://codepen.io/soniiic/pen/zEgrGb

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

Thanks for updating to a boxHeight option! Is it possible to add unit tests for this? Also, could you update the docs in https://github.com/chartjs/Chart.js/blob/master/docs/configuration/legend.md

@dan-sc
Copy link

dan-sc commented Oct 26, 2017

Ok, I'll have a go at those

@@ -96,6 +96,12 @@ module.exports = function(Chart) {
labelOpts.boxWidth;
}

function getBoxHeight(labelOpts, fontSize) {
return labelOpts.boxHeight ?
Copy link
Member

Choose a reason for hiding this comment

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

If we consider 0 a valid height (which I think is), then:

return helpers.isNullOrUndef(options.boxHeight) ? options.boxHeight : fontSize;

else this can be simplified as:

return options.boxHeight || fontSize;

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to handle labelOpts.usePointStyle as in getBoxWidth()? or is it unrelated?

@benmccann benmccann added this to the Version 2.8 milestone Nov 1, 2017
@benmccann
Copy link
Contributor

@dan-schoolcomms will you be able to address Simon's comments?

@soniiic
Copy link
Author

soniiic commented Nov 13, 2017

@benmccann Sorry for not keeping on top of this. Adding the unit tests would require me getting a development environment set up which at the moment I do not have as I have just made the edit within github itself. At the moment I don't have the time to do this, but I will see if I can get an hour or two to start it to see how far I get.

@etimberg
Copy link
Member

Closing due to inactivity

@etimberg etimberg closed this Dec 31, 2017
@simonbrunel simonbrunel removed this from the Version 2.8 milestone Jan 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants