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

Implement legend.align: 'start'/'center'/'end' #6141

Merged
merged 5 commits into from Mar 23, 2019

Conversation

dkichler
Copy link
Contributor

@dkichler dkichler commented Mar 18, 2019

  • New config parameter options.legend.align, for controlling alignment of legend blocks in horizontal/vertical legends.
  • Maintains backward compatibility for legends positioned on the left/right by defaulting to 'start'.
  • Replacing nearby pixel unit tests for legend with image based tests
  • Documentation for options.legend.align

Does not cover the full scope of @simonbrunel suggestions.

Covers only start/center/end of options.legend.align. Maintains backward compatibility for left/right positioned legends (defaulting to start).

Omits for now the stretch option, and all of the options.legend.labels.align options, which could follow as an extension.

Closes #5866
Fixes #3175

- New config parameter options.legend.align, for controlling alignment of legend blocks in horizontal/vertical legends.
- Maintains backward compatibility for legends positioned on the left/right by defaulting to 'start'.
- Replacing nearby pixel unit tests for legend with image based tests
- Documentation for options.legend.align
@dkichler
Copy link
Contributor Author

🤔 The failed CI build seems to be failing to load the newly introduced image tests. I'm unable to reproduce the failures locally. Admittedly, I have not attempted to reproduce exactly the build environment as far as Node and NPM version (I'm running Node v11.9.0 and npm 6.8.0).

@kurkle

This comment has been minimized.

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Couple of nit's

src/plugins/plugin.legend.js Outdated Show resolved Hide resolved
- whitespace as tabs only to align with project lint standards
- replacing 'this' references with a local 'me' variable
- removal of label text to avoid cross-platform font issues in image tests
- added an image test for default alignment of 'start' for vertical legends (left/right)
@dkichler
Copy link
Contributor Author

Aha, of course - cross-platform font differences would do it. Thanks for pointing that out @kurkle.

@dkichler
Copy link
Contributor Author

Feedback from the first round of review has been addressed.

@dkichler
Copy link
Contributor Author

I will add that I made efforts to ensure backward compatibility in hopes that this feature could be released in an upcoming minor rather than having to wait for another major.

@simonbrunel
Copy link
Member

Isn't it the same as #5866?

@simonbrunel simonbrunel changed the title Fixes Issue #3175: Implement legend.align: 'start'/'center'/'end' Mar 19, 2019
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

@dkichler Looks really good.

To make tests more robust / stable, I would add an explicit background (primitive) color (e.g. #00ff00) instead of the semi transparent gray one and remove the white border for the legend items and the doughnut sections (I think borderWidth: 0 is enough).

src/plugins/plugin.legend.js Outdated Show resolved Hide resolved
src/plugins/plugin.legend.js Outdated Show resolved Hide resolved
src/plugins/plugin.legend.js Outdated Show resolved Hide resolved
etimberg
etimberg previously approved these changes Mar 19, 2019
@dkichler
Copy link
Contributor Author

Will update when I find a few minutes with a commit covering the following feedback:

  • fix image test cases that still have text labels included in the test image
  • center will be the default alignment across all legend positions

- adding colour and removing borders from image tests for legend blocks.
- corrected/adjusted all images in image tests
- changed default align for vertical positioned legends to 'center'
- refactored alignment offset function
- removed superfluous 'line' variable
- made consistent the check for moving to a new line/column between vertical/horizontal fit algorithm
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Should we have tests for right/bottom too?

@dkichler
Copy link
Contributor Author

dkichler commented Mar 20, 2019

Having exhaustive tests is rarely a bad thing, unless they take forever to run, which these don't.

kurkle
kurkle previously approved these changes Mar 20, 2019
@@ -253,9 +255,9 @@ var Legend = Element.extend({
var boxWidth = getBoxWidth(labelOpts, fontSize);
var width = boxWidth + (fontSize / 2) + ctx.measureText(legendItem.text).width;

if (i === 0 || lineWidths[lineWidths.length - 1] + width + labelOpts.padding > minSize.width) {
if (i === 0 || lineWidths[lineWidths.length - 1] + width + labelOpts.padding > minSize.width - labelOpts.padding) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it's a bit funny to me that we have padding on both sides of the comparison. perhaps it should be

if (i === 0 || lineWidths[lineWidths.length - 1] + width + 2 * labelOpts.padding > minSize.width) {

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 agree - the fit logic was not trivial to follow. My intention was to make as few changes as possible while making this check consistent between vertical/horizontal. Ideally this fit algo would be refactored in its entirety to be more consistent as a whole between the two. I didn't really feel that was within the scope of this PR, but I could probably find a happy medium.

@dkichler
Copy link
Contributor Author

Should I squash this prior to merging? Would hate to negate all the approved reviews, but...

@simonbrunel
Copy link
Member

No, we will squash on merge.

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
New `options.legend.align`config option for controlling alignment of legend blocks in horizontal/vertical legends.
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.

Pie Graph legend on left or right is not vertically centered.
9 participants