feat(legend): add point shape styles to legend item #1227

rshen91 · 2021-06-29T20:50:18Z

Summary

By assigning a pointStyle.shape to a line, bubble or area series, legend item icons now show the appropriate shape of the markers. For instance, if a user specifies a hollow square shape, that is what will render for the legend marker for that series. By default, the legend icons are filled.

Details

Subsequent PRs will handle the stroke width computations and potential styling where the icon would not be filled or have unique styling (such as the marker on the series being one color, the stroke border being another color, then the icon in the legend could reflect this).

Issues

Closes #1081

Checklist

The proper chart type label was added (e.g. :xy, :partition) if the PR involves a specific chart type
The proper feature label was added (e.g. :interactions, :axis) if the PR involves a specific chart feature
Whenever possible, please check if the closing issue is connected to a running GH project
Proper documentation or storybook story was added for features that require explanation or tutorials
Unit tests were updated or added to match the most common scenarios

monfera

I haven't runtime tested; please consider some code changes, see the comments

packages/charts/src/chart_types/xy_chart/legend/legend.ts

packages/charts/src/components/icons/icon.tsx

markov00

I took a look at the point shapes example and looks like that square doesn't support the fill property as specified in the chart. We should fully align the shapes styles to the one in the chart. If the square has a red border and a yellow fill, we should reflect this in the legend

rshen91 · 2021-07-08T13:52:31Z

I took a look at the point shapes example and looks like that square doesn't support the fill property as specified in the chart. We should fully align the shapes styles to the one in the chart. If the square has a red border and a yellow fill, we should reflect this in the legend

This makes a lot of sense to me - I have removed the fill for the square icon in this commit 37cf9c1

packages/charts/src/chart_types/xy_chart/legend/legend.ts

packages/charts/src/chart_types/xy_chart/state/selectors/compute_legend.ts

packages/charts/src/common/legend.ts

packages/charts/src/components/legend/legend_item.tsx

packages/charts/src/components/icons/icon.tsx

nickofthyme

Ok sorry for all the back and forth. I think this is good to merge now. We can discuss my changes tomorrow and modify them as you see fit.

packages/charts/src/chart_types/xy_chart/renderer/shapes_paths.ts

nickofthyme · 2021-07-21T19:21:43Z

packages/charts/src/components/legend/legend_item.tsx

@@ -196,6 +206,7 @@ export class LegendListItem extends Component<LegendItemProps, LegendItemState>
            isSeriesHidden={isSeriesHidden}
            hasColorPicker={hasColorPicker}
            onClick={this.handleColorClick(hasColorPicker)}
+            pointStyle={alignLegendPointStyles ? pointStyle : undefined}


Used to explicitly control legend point style alignment by user. We could infer when to apply the styles as a fallback by comparing the local [area/line/bubble]SeriesStyles.point to the respective value on the Theme. This could be something like...

deepEqual(areaSeriesStyles.point, theme.area.point, true); // true for partial comparison

But that seems a bit much for now.

packages/charts/src/components/legend/legend.tsx

nickofthyme · 2021-07-21T20:26:06Z

packages/charts/src/components/legend/color.tsx

@@ -46,14 +46,14 @@ export const Color = memo(
            title="change series color"
            ref={ref}
          >
-            <Icon type="dot" color={color} aria-label={`Change series color, currently ${color}`} />
+            <LegendIcon pointStyle={pointStyle} color={color} ariaLabel={`Change series color, currently ${color}`} />


I'm not sure about this in the future. When the point styles take over this could mean the series color being selected/set would not be reflected in the color icon. Should revisit at a later time.

rshen91 · 2021-07-22T15:48:58Z

@markov00 @nickofthyme updated the vrts in 46ab4e4. The large file change is because of the styling for the pointStyles are taken from the theme by default with the empty fill. Let me know if this looks good to you thanks!

rshen91 · 2021-07-22T16:22:43Z

@elastic/eui-design do you mind providing a review on this PR please?

rshen91 · 2021-07-22T16:45:00Z

jenkins test this please

nickofthyme · 2021-07-22T18:27:04Z

@miukimiu the basic changes here is that for any area, line or bubble chart that has visible points we use the point styles on the legend icon. This would mean all area, line and bubble series styles now have hollow points with white fill. 👇🏼

If this is not desirable we could only apply the styles to the legend icon with they differ from the main them styles.

monfera

tl;dr using solid colored shapes would help readability and accessibility, for when there's no solid/holey shape ambiguity on a given chart. WCAG guidelines don't sufficiently address, yet still acknowledge the impact of shape size and aspect ratio when it comes to distinguishing colors or contrast. An example: given the same bounding box for the marker in the legend, the full paint of that bounding box will be more ink than a solid disk in that bounding box, which is still better than, say, an empty circle with a thin border.

Thanks for letting in a late comment after the Kibana testing week and PTO. I'd still like to squeeze in a question relating to this as a significant BREAKING change, specifically, the mass changes to mocks due to solid to hollow markers, as those can't be solved well with a follow-up PR.

If there's a variety of markers, it's indeed great to give the user a marker shape cue, as in this change, which is good for cross-referencing and accessibility:

Yet most charts, thankfully, don't have a plethora of marker types. In fact it's often unhelpful to mix marker types, as

on line charts, it's redundant, and the markers themselves introduce noise (see the above example for this as well - it's exaggerated, but real line charts will turn to such barbed wire charts easily)
on scatterplots, each marker type has different characteristics (ink surface area; ink bounding box; occlusion effects; crowding effects; perceptibility, saliency etc.) so a medium or high overplotting scatterplot will be a bit of a mess
etc

Mixing marker types can be great for aiding accessibility, if the data and chart configuration are conducive to it (not too many lines; not too large, yet discernible markers etc) as a redundant way to disambiguate lines, but then the same shape (eg. empty circle) shouldn't be reused with diverse colors.

The upshot is that most of the charts won't need legend disambiguation for marker shape.

Using holey markers (circle instead of disk etc.) has the negative that the legend ink area is diminished to a fraction of it, which reduces the ability to find the right legend item, discern colors etc. and is thus an accessibility issue. Even if the shape is holey on the line, it's easier to discern the color of a long line than of a small circle. Even if we disregard it, it's useful if at least the legend colors have significant "meat", for easier cross-referencing.

Proposal:

use solid markers in the legend if there's no ambiguity about holey vs solid markers on a given chart
even if the user opts for some other marker, eg. x everywhere on a chart, use solid colors in the legend (*)
if it's needed, please point a well discussed use case (eg. issue comments, PR comments here and conclusions) and add an option for enforcing the behavior currently in this PR commit; eg. which user team would need holey legends on non-ambiguous charts, and why

(*) Comments for where it might be heading, post this PR:

Currently, the legends area is a singleton on the chart. Therefore the encoded information of the markers are conflated. For example, it's currently not possible to have three distinct legends for color, marker type and size. This impairs readability of each encoding channel. So the long term thing is to treat legends as data visualizations in and of themselves (a recurring theme) so that a legend is not a singleton on the chart, and also, so that multiple charts can share some legend(s).

In light of such future, we can restate my request this way: when there's no marker ambiguity, let's NOT EVEN try to use the legend swatch to convey anything about marker shape; it's just a color swatch.

Also, having decoupled the marker shape from the color assignment for non-ambiguous cases, we can do better and switch to squares or rectangles for the color swatch (with or without rounded corner) to further increase the color ink area.

monfera · 2021-07-26T10:21:34Z

packages/charts/src/chart_types/xy_chart/legend/legend.ts

+function getPointStyle(spec: BasicSeriesSpec, theme: Theme): PointStyle | undefined {
+  const mergeOptions: MergeOptions = { mergeOptionalPartialValues: true };
+  if (isBubbleSeriesSpec(spec)) {
+    return mergePartial(theme.bubbleSeriesStyle.point, spec.bubbleSeriesStyle?.point, mergeOptions);
+  } else if (isLineSeriesSpec(spec)) {
+    return mergePartial(theme.lineSeriesStyle.point, spec.lineSeriesStyle?.point, mergeOptions);
+  } else if (isAreaSeriesSpec(spec)) {
+    return mergePartial(theme.areaSeriesStyle.point, spec.areaSeriesStyle?.point, mergeOptions);
+  }
+}


It's not a PR comment, yet a great example for how TS leads to hoops we have to jump through (in the code, as well as runtime) to satisfy its type verification operations. TS requires a specific, and sometimes artificial coding style. Maybe we could eventually move the series type to distinct classes, as class instances have both static and runtime type, and something like this if cascade would become a simple method call

rshen91 · 2021-07-26T15:24:19Z

For the scope of this PR, after discussing with the team, we will make sure the legend icon is filled in all cases.

Subsequent PRs can handle the stroke width computations and potential styling where the icon would not be filled or have unique styling (such as the marker on the series being one color, the stroke border being another color, then the icon in the legend could reflect this). I opened issue 1267 for tracking discussions beyond this PR. Thanks!

markov00

Changes looks good to me!

# [33.1.0](v33.0.2...v33.1.0) (2021-07-28) ### Bug Fixes * persisted color via color picker ([#1265](#1265)) ([4205a7f](4205a7f)) ### Features * **legend:** add point shape styles to legend item ([#1227](#1227)) ([46be1d1](46be1d1)) * **partition:** waffle chart ([#1255](#1255)) ([156662a](156662a))

nickofthyme · 2021-07-28T16:49:17Z

🎉 This PR is included in version 33.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

rshen91 added 3 commits June 29, 2021 14:47

feat: add shape to legend item

414fa37

Merge remote-tracking branch 'upstream/master' into shape-legend

10f447b

test: update tests

d779b28

monfera reviewed Jul 1, 2021

View reviewed changes

packages/charts/src/chart_types/xy_chart/legend/legend.ts Outdated Show resolved Hide resolved

packages/charts/src/components/icons/icon.tsx Outdated Show resolved Hide resolved

rshen91 added 8 commits July 1, 2021 06:49

feat: add point style type check for area and line series

a68797c

test: add story

bf69b05

Merge branch 'master' into shape-legend

9d26b1a

Merge branch 'master' into shape-legend

95cac10

Merge remote-tracking branch 'upstream/master' into shape-legend

7e2d1f5

Merge remote-tracking branch 'origin/shape-legend' into shape-legend

badbef6

test: update stories

5b847e4

fix: update shapes per chart theme as fallback

bfaa207

markov00 reviewed Jul 8, 2021

View reviewed changes

rshen91 added 4 commits July 8, 2021 07:22

Merge remote-tracking branch 'upstream/master' into shape-legend

c03bf6c

Merge branch 'master' into shape-legend

5ebd342

Merge remote-tracking branch 'origin/shape-legend' into shape-legend

2040290

fix: remove the fill for the square icon

37cf9c1

nickofthyme reviewed Jul 8, 2021

View reviewed changes

feat: refactor to allow all point styles

957d817

rshen91 marked this pull request as ready for review July 8, 2021 18:14

rshen91 changed the title ~~feat(legend): add shape to legend item~~ feat(legend): add point shape styles to legend item Jul 8, 2021

rshen91 added :legend Legend related issue :xy Bar/Line/Area chart related labels Jul 8, 2021

rshen91 added 6 commits July 8, 2021 14:21

fix: remove unused icon files

4c47d58

fix: add default check of seriesTypes for vrt

59e649d

test: update test snapshots

f079e8c

fix: update chart api

fedf375

fix: fix tests and circle size

10a2825

fix: catch filled circles for legend

b57c8f4

nickofthyme added 2 commits July 21, 2021 13:35

feat: allow partial compare with deepEqual with tests

07fde21

test: fix broken tests, update vrts per changes

c3219d1

nickofthyme approved these changes Jul 21, 2021

View reviewed changes

nickofthyme added 2 commits July 21, 2021 14:29

fix: minor math check

db4bbf0

fix: legend icon with color picker

34b3012

nickofthyme reviewed Jul 21, 2021

View reviewed changes

rshen91 and others added 3 commits July 22, 2021 08:15

test: update tests

537b375

refactor: remove align prop in favor of always applying styles

a9feb73

test: update vrts to take on styling

46ab4e4

elizabetdev self-requested a review July 22, 2021 17:51

monfera suggested changes Jul 26, 2021

View reviewed changes

monfera reviewed Jul 26, 2021

View reviewed changes

test: finalize scope and vrts

71c00ed

rshen91 requested a review from monfera July 26, 2021 17:06

rshen91 mentioned this pull request Jul 26, 2021

Legend icons taking on more customization styling #1267

Open

rshen91 added 2 commits July 26, 2021 11:19

test: update snapshot unit tests

47702ab

test: update vrt

f2d780b

monfera approved these changes Jul 27, 2021

View reviewed changes

markov00 approved these changes Jul 27, 2021

View reviewed changes

rshen91 merged commit 46be1d1 into elastic:master Jul 27, 2021

rshen91 deleted the shape-legend branch July 27, 2021 13:33

formgeist mentioned this pull request Jul 27, 2021

[APM] Align instances legend item style to the chart elastic/kibana#106851

Open

nickofthyme added the released Issue released publicly label Jul 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(legend): add point shape styles to legend item #1227

feat(legend): add point shape styles to legend item #1227

rshen91 commented Jun 29, 2021 •

edited

Loading

monfera left a comment

markov00 left a comment

rshen91 commented Jul 8, 2021

nickofthyme left a comment

nickofthyme Jul 21, 2021

nickofthyme Jul 21, 2021

rshen91 commented Jul 22, 2021

rshen91 commented Jul 22, 2021

rshen91 commented Jul 22, 2021

nickofthyme commented Jul 22, 2021 •

edited

Loading

monfera left a comment •

edited

Loading

monfera Jul 26, 2021

rshen91 commented Jul 26, 2021 •

edited

Loading

markov00 left a comment

nickofthyme commented Jul 28, 2021

feat(legend): add point shape styles to legend item #1227

feat(legend): add point shape styles to legend item #1227

Conversation

rshen91 commented Jun 29, 2021 • edited Loading

Summary

Details

Issues

Checklist

monfera left a comment

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

rshen91 commented Jul 8, 2021

nickofthyme left a comment

Choose a reason for hiding this comment

nickofthyme Jul 21, 2021

Choose a reason for hiding this comment

nickofthyme Jul 21, 2021

Choose a reason for hiding this comment

rshen91 commented Jul 22, 2021

rshen91 commented Jul 22, 2021

rshen91 commented Jul 22, 2021

nickofthyme commented Jul 22, 2021 • edited Loading

monfera left a comment • edited Loading

Choose a reason for hiding this comment

monfera Jul 26, 2021

Choose a reason for hiding this comment

rshen91 commented Jul 26, 2021 • edited Loading

markov00 left a comment

Choose a reason for hiding this comment

nickofthyme commented Jul 28, 2021

rshen91 commented Jun 29, 2021 •

edited

Loading

nickofthyme commented Jul 22, 2021 •

edited

Loading

monfera left a comment •

edited

Loading

rshen91 commented Jul 26, 2021 •

edited

Loading