Skip to content

fix: group legend items by label and color#999

Merged
markov00 merged 13 commits intoelastic:masterfrom
markov00:2021_01_27-fix_small_multiples_legend
Feb 16, 2021
Merged

fix: group legend items by label and color#999
markov00 merged 13 commits intoelastic:masterfrom
markov00:2021_01_27-fix_small_multiples_legend

Conversation

@markov00
Copy link
Copy Markdown
Collaborator

@markov00 markov00 commented Jan 30, 2021

Summary

This PR fixes the duplicated legend elements when displaying small multiples.
The legend item concept should allow the user to uniquely link a geometry color to a label. There are situation where multiple geometries or series are colored with the same color, like in a small multiple, and all of these series are related to the same metric for example. This PR removes any duplicate of the key color, label in the legend item and introduce the ability to have multiple series referenced by the same legend item.

BREAKING CHANGE:
The LegendActionProps and the LegendColorPickerProps, used to add actions and color picker through the legend now receive an array of SeriesIdentifiers instead of a single one:

export interface LegendActionProps {
    color: string;
    label: string;
    series: SeriesIdentifier[]; // <--- this was changed from a single SeriesIndentifier
}
export interface LegendColorPickerProps {
    anchor: HTMLElement;
    color: Color;
    onChange: (color: Color | null) => void;
    onClose: () => void;
    seriesIdentifiers: SeriesIdentifier[]; // <--- this was changed from a single SeriesIndentifier, the prop name was also changed to plural
}

Checklist

Delete any items that are not applicable to this PR.

  • Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
  • 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

@markov00 markov00 added enhancement New feature or request :legend Legend related issue :xy Bar/Line/Area chart related :small multiples Small multiples/trellising related issues labels Jan 30, 2021
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 30, 2021

Codecov Report

Merging #999 (2fc5a16) into master (514466f) will increase coverage by 0.65%.
The diff coverage is 76.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #999      +/-   ##
==========================================
+ Coverage   72.13%   72.78%   +0.65%     
==========================================
  Files         355      363       +8     
  Lines       11160    11211      +51     
  Branches     2449     2434      -15     
==========================================
+ Hits         8050     8160     +110     
+ Misses       3095     3037      -58     
+ Partials       15       14       -1     
Flag Coverage Δ
unittests 72.78% <76.86%> (+0.65%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rt_types/heatmap/state/selectors/compute_legend.ts 40.00% <ø> (ø)
src/chart_types/partition_chart/layout/config.ts 54.54% <ø> (ø)
...types/partition_chart/layout/types/config_types.ts 100.00% <ø> (ø)
...es/partition_chart/layout/types/viewmodel_types.ts 83.33% <ø> (ø)
...partition_chart/renderer/dom/highlighter_hover.tsx 44.44% <0.00%> (ø)
...artition_chart/renderer/dom/highlighter_legend.tsx 44.44% <0.00%> (ø)
...tition_chart/state/selectors/is_tooltip_visible.ts 77.77% <0.00%> (+14.14%) ⬆️
src/specs/settings.tsx 90.32% <ø> (ø)
...on_chart/state/selectors/get_highlighted_shapes.ts 58.33% <16.66%> (+26.19%) ⬆️
...t_types/partition_chart/state/selectors/tooltip.ts 45.45% <16.66%> (+16.28%) ⬆️
... and 61 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1abb85...2fc5a16. Read the comment docs.

@nickofthyme
Copy link
Copy Markdown
Collaborator

nickofthyme commented Feb 1, 2021

I feel like this approach will suffice in the short term but it seems like this solution could be problematic. Mainly around grouping by color and not some arbitrary value.

So in vislib if someone changes the color of the series to the same value those series get combined which is only reversible by clearing the colors for the combined series.

Screen Recording 2021-02-01 at 01 03 PM

Grid color picker example code
/*
 * Licensed to Elasticsearch B.V. under one or more contributor
 * license agreements. See the NOTICE file distributed with
 * this work for additional information regarding copyright
 * ownership. Elasticsearch B.V. licenses this file to you under
 * the Apache License, Version 2.0 (the "License"); you may
 * not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 * http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing,
 * software distributed under the License is distributed on an
 * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
 * KIND, either express or implied.  See the License for the
 * specific language governing permissions and limitations
 * under the License.
 */
import { EuiWrappingPopover, EuiColorPicker, EuiSpacer, EuiFlexItem, EuiButtonEmpty, EuiButton } from '@elastic/eui';
import { action } from '@storybook/addon-actions';
import { boolean } from '@storybook/addon-knobs';
import { DateTime } from 'luxon';
import React, { useMemo, useState } from 'react';

import {
  ScaleType,
  Position,
  Chart,
  Axis,
  LineSeries,
  GroupBy,
  SmallMultiples,
  Settings,
  LIGHT_THEME,
  niceTimeFormatByDay,
  timeFormatter,
  LegendColorPicker,
} from '../../src';
import { SeriesKey } from '../../src/common/series_id';
import { SeededDataGenerator } from '../../src/mocks/utils';
import { Color, toEntries } from '../../src/utils/common';
import { SB_SOURCE_PANEL } from '../utils/storybook';

const dg = new SeededDataGenerator();
const numOfDays = 90;
const groupNames = new Array(16).fill(0).map((d, i) => String.fromCharCode(97 + i));
const data = dg.generateGroupedSeries(numOfDays, 16).map((d) => {
  return {
    y: d.y,
    x: DateTime.fromISO('2020-01-01T00:00:00Z').plus({ days: d.x }).toMillis(),
    g: d.g,
    h: `host ${groupNames.indexOf(d.g) % 4}`,
    v: `metric ${Math.floor(groupNames.indexOf(d.g) / 4)}`,
  };
});

const onChangeAction = action('onChange');
const onCloseAction = action('onClose');

export const Example = () => {
  const showLegend = boolean('Show Legend', true);
  const onElementClick = action('onElementClick');

  const [colors, setColors] = useState<Record<SeriesKey, Color | null>>({});
  const CustomColorPicker: LegendColorPicker = useMemo(
    () => ({ anchor, color, onClose, seriesIdentifiers, onChange }) => {
      const handleClose = () => {
        onClose();
        onCloseAction();
        setColors((prevColors) => ({
          ...prevColors,
          ...toEntries(seriesIdentifiers, 'key', color),
        }));
      };
      const handleChange = (c: Color | null) => {
        setColors((prevColors) => ({
          ...prevColors,
          ...toEntries(seriesIdentifiers, 'key', c),
        }));
        onChange(c);
        onChangeAction(c);
      };

      return (
        <>
          <EuiWrappingPopover isOpen button={anchor} closePopover={handleClose} anchorPosition="leftCenter" ownFocus>
            <EuiColorPicker display="inline" color={color} onChange={handleChange} />
            <EuiSpacer size="m" />
            <EuiFlexItem grow={false}>
              <EuiButtonEmpty size="s" onClick={() => handleChange(null)}>
                Clear color
              </EuiButtonEmpty>
            </EuiFlexItem>
            <EuiButton fullWidth size="s" onClick={handleClose}>
              Done
            </EuiButton>
          </EuiWrappingPopover>
        </>
      );
    },
    [setColors],
  );
  CustomColorPicker.displayName = 'CustomColorPicker';

  console.log(colors);

  return (
    <Chart className="story-chart">
      <Settings
        legendColorPicker={CustomColorPicker}
        onElementClick={onElementClick}
        showLegend={showLegend}
        theme={{
          lineSeriesStyle: {
            point: {
              visible: false,
            },
          },
        }}
      />
      <Axis
        id="time"
        title="timestamp"
        position={Position.Bottom}
        gridLine={{ visible: true }}
        ticks={2}
        style={{
          tickLabel: {
            padding: 10,
          },
          axisTitle: {
            padding: 0,
          },
          tickLine: {
            visible: false,
          },
          axisLine: {
            visible: false,
          },
        }}
        tickFormat={timeFormatter(niceTimeFormatByDay(numOfDays))}
      />
      <Axis
        id="y"
        title="metric"
        position={Position.Left}
        gridLine={{ visible: true }}
        domain={{
          max: 10,
        }}
        ticks={2}
        style={{
          axisTitle: {
            padding: 0,
          },
          tickLabel: {
            padding: 5,
          },
          tickLine: {
            visible: false,
          },
          axisLine: {
            visible: false,
          },
        }}
        tickFormat={(d) => d.toFixed(2)}
      />

      <GroupBy
        id="v_split"
        by={(spec, { v }) => {
          return v;
        }}
        sort="alphaAsc"
      />
      <GroupBy
        id="h_split"
        by={(spec, { h }) => {
          return h;
        }}
        sort="alphaAsc"
      />
      <SmallMultiples
        splitVertically="v_split"
        splitHorizontally="h_split"
        style={{ verticalPanelPadding: [0, 0.3] }}
      />

      <LineSeries
        id="line"
        xScaleType={ScaleType.Time}
        yScaleType={ScaleType.Linear}
        timeZone="UTC"
        xAccessor="x"
        yAccessors={['y']}
        color={({ smHorizontalAccessorValue, key }) => {
          const val = Number(`${smHorizontalAccessorValue}`.split('host ')[1]);
          return colors[key] ?? LIGHT_THEME.colors.vizColors[val];
        }}
        data={data}
      />
    </Chart>
  );
};
Example.story = {
  parameters: {
    options: { selectedPanel: SB_SOURCE_PANEL },
    info: {
      text: `It is possible to add either a vertical and horizontal \`<GroupBy/>\` operations to create a grid of
small multiples.
The assignment of the series colors can be handled by defining an accessor in the \`color\` prop of the series that
consider the \`smHorizontalAccessorValue\` or \`smVerticalAccessorValue\` values when returning the assigned color.
`,
    },
  },
};

@markov00
Copy link
Copy Markdown
Collaborator Author

markov00 commented Feb 2, 2021

@nickofthyme thanks for the example, you are right, a situation like this can create issues in the future.
Let me think about how we can fix this

insertIndex: i,
}));

const smallMultipleUniqueValues = dataSeries.reduce<{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great general code changes in your PR! Besides replacing the let, also avoids the recreation of the Sets and nicely scopes things into a tight block

@nickofthyme
Copy link
Copy Markdown
Collaborator

@nickofthyme thanks for the example, you are right, a situation like this can create issues in the future.
Let me think about how we can fix this

@markov00 the first thing that comes to my mind is just a unique string to group the series by, that is not tied to any property. Not sure how that works implementation-wise but thought I'd put it out there.

Base automatically changed from master to masterx February 2, 2021 23:10
Base automatically changed from masterx to master February 2, 2021 23:28
@nickofthyme
Copy link
Copy Markdown
Collaborator

I tested this in kibana to implement in the vislib replacement and everything seems to work as expected.

Screen Recording 2021-02-09 at 08 53 48 PM

One thing I am not so sure about is the multiple seriesIdentifiers on the legend actions. I wonder how this would be used to change the colors and filter actions. I will look at this tomorrow but I'm sure there is a way to achieve it.

Copy link
Copy Markdown
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Looks good to me, just left a few comments to address and I'll approve. Also tested storybook locally on Chrome and saw no issues.

Copy link
Copy Markdown
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

The latest code changes look good to me.

@markov00 markov00 merged commit 5d32f23 into elastic:master Feb 16, 2021
@markov00 markov00 deleted the 2021_01_27-fix_small_multiples_legend branch February 16, 2021 17:33
github-actions bot pushed a commit that referenced this pull request Feb 16, 2021
# [25.0.0](v24.6.0...v25.0.0) (2021-02-16)

### Bug Fixes

* group legend items by label and color ([#999](#999)) ([5d32f23](5d32f23))

### Features

* **axis:** log scale improvements and options ([#1014](#1014)) ([0f52688](0f52688))

### BREAKING CHANGES

* The `LegendActionProps` and the `LegendColorPickerProps`, used to add actions and color picker through the legend now receive an array of `SeriesIdentifiers`
@markov00
Copy link
Copy Markdown
Collaborator Author

🎉 This PR is included in version 25.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Feb 16, 2021
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [25.0.0](elastic/elastic-charts@v24.6.0...v25.0.0) (2021-02-16)

### Bug Fixes

* group legend items by label and color ([opensearch-project#999](elastic/elastic-charts#999)) ([ce0bfba](elastic/elastic-charts@ce0bfba))

### Features

* **axis:** log scale improvements and options ([opensearch-project#1014](elastic/elastic-charts#1014)) ([8bac5e8](elastic/elastic-charts@8bac5e8))

### BREAKING CHANGES

* The `LegendActionProps` and the `LegendColorPickerProps`, used to add actions and color picker through the legend now receive an array of `SeriesIdentifiers`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request :legend Legend related issue released Issue released publicly :small multiples Small multiples/trellising related issues :xy Bar/Line/Area chart related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants