Implement legend.align: 'start'/'center'/'end' #6141
Conversation
- 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
|
🤔 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 |
This comment has been minimized.
This comment has been minimized.
test/fixtures/plugin.legend/legend-doughnut-left-start-mulitiline.json
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)
|
Aha, of course - cross-platform font differences would do it. Thanks for pointing that out @kurkle. |
|
Feedback from the first round of review has been addressed. |
|
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. |
|
Isn't it the same as #5866? |
simonbrunel
changed the title
There was a problem hiding this comment.
@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).
|
Will update when I find a few minutes with a commit covering the following feedback:
|
- 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
|
Having exhaustive tests is rarely a bad thing, unless they take forever to run, which these don't. |
src/plugins/plugin.legend.js
Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
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) {
There was a problem hiding this comment.
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.
…onsistent and slightly more readable
|
Should I squash this prior to merging? Would hate to negate all the approved reviews, but... |
|
No, we will squash on merge. |
New `options.legend.align`config option for controlling alignment of legend blocks in horizontal/vertical legends.
Does not cover the full scope of @simonbrunel suggestions.
Covers only
start/center/endofoptions.legend.align. Maintains backward compatibility for left/right positioned legends (defaulting tostart).Omits for now the
stretchoption, and all of theoptions.legend.labels.alignoptions, which could follow as an extension.Closes #5866
Fixes #3175