Add legend label style option #5622

nagix · 2018-07-07T13:32:21Z

This feature was proposed several times in the past (#4163, #4496, #4811 and #4890) in order to use a line or a custom-sized box as a legend for a line in line charts, but no PR has been merged yet. I would like to try a bit different approach.

Deprecate the usePointStyle legend label option
Instead, introduce the style legend label option, which can have 'box', 'line' and 'point' value
- 'box': The same appearance as the current implementation
- 'line': The line style is used. Border width, border color, line cap style, line join style and line dashes are inherited from the corresponding dataset
- 'point': The same appearance as the current usePointStyle option.
- If not set, the 'line' style is used for line elements, and the 'box' style for other elements.
As it detects the dataset type and choose a suitable legend label style, mixed charts are also supported.

See https://jsfiddle.net/nagix/d86rvwn5/

Note that the chart with style: 'point' shows a dashed circle, but this should not be a dashed line. I'm trying to fix this with #5621.

The existing tests are fixed more tests are added. Also, document is updated.

Fixes #4727

- Add style option to legend labels config suppoting box, line and point style - Deprecate usePointStyle option - BoxWidth is now calculated for each legend item as it can have different width - Fix the existing tests and add more tests - Update document

simonbrunel · 2018-07-07T15:22:38Z

I'm not sure we should deprecate usePointStyle. IMO, this is 2 different features: labels.usePointStyle allows to pick the dataset point options (instead of the dataset line options) while labels.style allows to control the shape of the label.

I think the following use cases should be valid for a line or radar chart:

style: 'point' and usePointStyle: false: draw points with the line color/border/...
style: 'point' and usePointStyle: true: draw points with the point color/border/...
style: 'box' and usePointStyle: true: draw boxes with the point color/border/...
style: 'line' and usePointStyle: true: draw lines with the point color/border/...
...

nagix · 2018-07-07T15:40:03Z

@simonbrunel usePointStyle: true doesn't mean using the point color/border/..., but using pointStyle shapes such as 'circle' and 'triangle'. So, style: 'box' and usePointStyle: true are exclusive, and style: 'point' is intended to what usePointStyle: true does. style: 'line' and usePointStyle: true can be used together and useful, though.

simonbrunel · 2018-07-07T15:50:09Z

I thought usePointStyle was also using the point color/border/... instead of the line ones.

Note that the chart with style: 'point' shows a dashed circle, but this should not be a dashed line. I'm trying to fix this with #5621.

I think it should be a dashed line in this case and I would not do special cases based on the chart type. If we want the labels to use the point color/border/ ... instead of the line options, then we should introduce a new option (if usePointStyle is not this one).

nagix · 2018-07-07T16:05:16Z

In the current implementation, usePointStyle: true doesn't use point color/border/... but use line color/border/..., and that causes inconsistency in appearance between legend and chart elements when they have different styles. But, this is for the other PR.

In this PR, I'm just focusing on "shapes". usePointStyle only switches between a box and point shape, but this proposal is trying to give more options including a line and possibly "lineWithPoint".

Hedva · 2018-07-09T12:09:50Z

Really appreciate this.

Only I would expect:

 datasets: [{
      type: 'line',

to be:

 datasets: [{
      labelType: 'line', //box, line, circle

And that the labelType property in the dataset, overrides whatever is set as default at options.legend.labels.style.

Edit: Never mind i was confused about type. I didn't realize it was for the combined line/bar chart.
But I still do think this is something you want to be able to adjust per dataset, with something like labelType.

etimberg · 2018-07-09T23:41:09Z

src/controllers/controller.doughnut.js


 						return {
 							text: label,
 							fillStyle: fill,
 							strokeStyle: stroke,
 							lineWidth: bw,
 							hidden: isNaN(ds.data[i]) || meta.data[i].hidden,
+							// `usePointStyle` is deprecated. To be removed at version 3
+							style: !style && labelOpts.usePointStyle ? 'point' : style,


Thanks for adding support for the older option

etimberg · 2018-07-09T23:43:26Z

test/specs/global.deprecations.tests.js

@@ -6,6 +6,32 @@ describe('Deprecations', function() {
 				expect(Chart.layoutService).toBe(Chart.layouts);
 			});
 		});
+
+		describe('Legend Labels: usePointStyle option', function() {


@simonbrunel should we put this inside a describe('Version 2.8.0', ...) block?

It's already inside 2.8.0 deprecations

simonbrunel · 2018-07-10T07:59:20Z

@nagix I totally get that this PR is not about color/border/etc. but #5621 uses usePointStyle to switch between the dataset (line) and element (point) colors/border/etc. I don't think style: 'point' should also change the color/border/etc.: the shape and the color/border/etc. should be independent IMO.

I don't really like the term style because it's too confusing. We don't know if we are talking about the shape, the colors, the opacity, ... or everything. We may prefer to call this new option: labels.shape or labels.symbol instead of labels.style.

So I would rather keep usePointStyle to make the legend label match the point style (shape/color/border/etc.). labels.shape (if defined) would override the point 'symbol' if usePointStyle: true while using the point color/border/etc.

Finally, labels.shape: 'point' would mean: use the current point shape (not the other styling options). Ideally, we should support all other shapes, especially circle since it's a wanted feature for any type of charts.

So, style: 'box' and usePointStyle: true are exclusive

It's not exclusive, it allows to use the point color/border/etc. while displaying a box, which I'm sure is a valid use case. The other way is also valid: shape: 'point', usePointStyle: false, meaning I want to use the line color/border/etc. while displaying the current point shape.

What do you guys think? (sorry for the long comment)

nagix · 2018-07-10T18:43:42Z

I agree that we keep usePointStyle to make the legend label symbol match the point style while we introduce symbol to control the type of the symbol. As the value 'point' doesn't represent the shape, I'd prefer the term symbol rather than shape. As @simonbrunel said, style is definitely confusing.

labels.shape (if defined) would override the point 'symbol' if usePointStyle: true while using the point color/border/etc.

Basically, this is fine. But, I would like to enable 'the point symbol on the line' when symbol: 'line' and usePointStyle: true as this style is quite common. So, I propose this:

Any comments?

simonbrunel · 2018-07-10T19:19:37Z

As the value 'point' doesn't represent the shape, I'd prefer the term symbol rather than shape

I would call the point shape (triangle, circle, etc.) a symbol (per #4811)

I don't see the necessity of the box or line symbol in the point style

I still think it's a valid use case, why enforcing such restriction?

--

Actually, #4811 is closer to what I'm thinking about customizing the legend labels: allow the user to pick any available symbol as legend labels (whatever the usePointStyle value). I'm not fan of complex / rigid option logic and prefer to keep things simple and flexible. At some point, someone will ask for circle or triangle in a bar chart.

So I think I prefer the new option to select / override the label symbol (any of this list) while usePointStyle switches between dataset/element style (I would maybe not support point since it doesn't make sense in all charts).

nagix · 2018-07-13T14:36:53Z

Ok, in that case, I can wait for #4811.

benmccann · 2018-07-25T14:48:50Z

@nagix I'm not completely understanding the conclusion that you and Simon came to. Is this PR a duplicate of #4811 and should be closed? Or only partially a duplicate and still adds some new functionality in which case it should be updated to add only the new functionality?

I'm hoping we can either update or close the PR. I'd like to make sure all the open PRs are in a reviewable state. Otherwise it gets really hard to keep track of which we need to review and which we shouldn't

benmccann · 2018-07-29T16:21:13Z

@nagix should this PR be updated or closed?

benmccann

Drop in favor of #4811 or update to include only functionality not addressed there

benmccann · 2018-08-03T04:57:10Z

@nagix I'm going to close this PR as inactive since there hasn't been any response and it's not clear to me from the comments that it's still needed. Please feel free to reopen if I'm wrong about that

Add legend label style option

4784ba2

- Add style option to legend labels config suppoting box, line and point style - Deprecate usePointStyle option - BoxWidth is now calculated for each legend item as it can have different width - Fix the existing tests and add more tests - Update document

nagix force-pushed the issue-5622 branch from 1abc242 to 4784ba2 Compare July 7, 2018 14:04

simonbrunel added the type: enhancement label Jul 7, 2018

etimberg requested a review from simonbrunel July 9, 2018 23:38

etimberg approved these changes Jul 9, 2018

View reviewed changes

jadurani mentioned this pull request Jul 23, 2018

Customize legend box height #2051

Closed

benmccann added the status: stale label Jul 29, 2018

benmccann suggested changes Jul 30, 2018

View reviewed changes

benmccann closed this Aug 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add legend label style option #5622

Add legend label style option #5622

nagix commented Jul 7, 2018 •

edited

simonbrunel commented Jul 7, 2018

nagix commented Jul 7, 2018 •

edited

simonbrunel commented Jul 7, 2018

nagix commented Jul 7, 2018 •

edited

Hedva commented Jul 9, 2018 •

edited

etimberg Jul 9, 2018

etimberg Jul 9, 2018

simonbrunel Jul 10, 2018

simonbrunel commented Jul 10, 2018

nagix commented Jul 10, 2018 •

edited

simonbrunel commented Jul 10, 2018

nagix commented Jul 13, 2018

benmccann commented Jul 25, 2018

benmccann commented Jul 29, 2018

benmccann left a comment

benmccann commented Aug 3, 2018

Add legend label style option #5622

Add legend label style option #5622

Conversation

nagix commented Jul 7, 2018 • edited

simonbrunel commented Jul 7, 2018

nagix commented Jul 7, 2018 • edited

simonbrunel commented Jul 7, 2018

nagix commented Jul 7, 2018 • edited

Hedva commented Jul 9, 2018 • edited

etimberg Jul 9, 2018

Choose a reason for hiding this comment

etimberg Jul 9, 2018

Choose a reason for hiding this comment

simonbrunel Jul 10, 2018

Choose a reason for hiding this comment

simonbrunel commented Jul 10, 2018

nagix commented Jul 10, 2018 • edited

simonbrunel commented Jul 10, 2018

nagix commented Jul 13, 2018

benmccann commented Jul 25, 2018

benmccann commented Jul 29, 2018

benmccann left a comment

Choose a reason for hiding this comment

benmccann commented Aug 3, 2018

nagix commented Jul 7, 2018 •

edited

nagix commented Jul 7, 2018 •

edited

nagix commented Jul 7, 2018 •

edited

Hedva commented Jul 9, 2018 •

edited

nagix commented Jul 10, 2018 •

edited