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

fix: group legend items by label and color #999

Merged

Conversation

markov00
Copy link
Member

@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

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
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
Member 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

@@ -449,12 +429,30 @@ export function getDataSeriesFromSpecs(
insertIndex: i,
}));

const smallMultipleUniqueValues = dataSeries.reduce<{
Copy link
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
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
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
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.

src/chart_types/xy_chart/legend/legend.ts Outdated Show resolved Hide resolved
src/specs/settings.tsx Outdated Show resolved Hide resolved
src/specs/settings.tsx Outdated Show resolved Hide resolved
src/state/actions/legend.ts Show resolved Hide resolved
src/utils/common.ts Show resolved Hide resolved
src/components/legend/legend_item.tsx Outdated Show resolved Hide resolved
stories/small_multiples/3_grid_lines.tsx Outdated Show resolved Hide resolved
stories/small_multiples/4_vertical_bars.tsx Outdated Show resolved Hide resolved
Copy link
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.

.playground/index.html Show resolved Hide resolved
@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
Member 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