refactor(legend): remove visibility button #252

markov00 · 2019-07-02T08:19:09Z

Summary

This PR removes the visibility button. The visibility change handling is applied on the legend item title.

BREAKING CHANGE: the onLegendItemClick click handler is no longer applied when clicking on the title. Instead a simple visibility change is applied.

Before:

After:

cc @cchaos
I've some doubts on this because removes two important features right now:

without the eye/eyeclosed icons we don't have a graphic representation that specify that a series is hidden. Is a ~~strikethrough~~ enough or is much more difficult to read?
There are few legend item interaction that we need to support:
- Hide/show series (currently the only interaction on TSVB, but not an interaction on vizlib)
- click to filter (on vizlib clicking on the legend item is used to create and apply a global filter)

Before merging I'd like to check with you @cchaos these things.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are 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)
This was checked for cross-browser compatibility, including a check against IE11
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
Each commit follows the convention

the visibility button is removed and the visibility change is applied on the legend item title BREAKING CHANGE: the `onLegendItemClick` click handler is no longer applied when clicking on the title. Instead a simple visibility change is applied.

codecov-io · 2019-07-02T08:23:15Z

Codecov Report

Merging #252 into master will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #252      +/-   ##
==========================================
+ Coverage   97.84%   97.91%   +0.06%     
==========================================
  Files          36       36              
  Lines        2693     3162     +469     
  Branches      607      790     +183     
==========================================
+ Hits         2635     3096     +461     
- Misses         51       55       +4     
- Partials        7       11       +4

Impacted Files	Coverage Δ
src/lib/series/stacked_series_utils.ts	`96.47% <0%> (-1.75%)`	⬇️
src/state/utils.ts	`96.28% <0%> (-0.16%)`	⬇️
src/lib/series/specs.ts	`100% <0%> (ø)`	⬆️
src/lib/series/series.ts	`100% <0%> (ø)`	⬆️
src/lib/series/domains/y_domain.ts	`100% <0%> (ø)`	⬆️
src/lib/axes/canvas_text_bbox_calculator.ts	`100% <0%> (ø)`	⬆️
src/lib/axes/axis_utils.ts	`100% <0%> (ø)`	⬆️
src/lib/utils/commons.ts	`100% <0%> (ø)`	⬆️
src/lib/series/rendering.ts	`98.06% <0%> (+0.15%)`	⬆️

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 af92736...e40a42d. Read the comment docs.

cchaos · 2019-07-02T12:41:51Z

without the eye/eyeclosed icons we don't have a graphic representation that specify that a series is hidden

What we do in Maps is replace the legend color indicator with the eyeClosed icon. We can do the same here though we may need to replace the icon with a smaller version since it's a bit overbearing compared to the dot.

There are few legend item interaction that we need to support:

Hide/show series (currently the only interaction on TSVB, but not an interaction on vizlib)

Is this not the current interaction by clicking on the legend item?

click to filter (on vizlib clicking on the legend item is used to create and apply a global filter)

Yes back when that functionality was actually in elastic-charts I had spoken with @emmacunningham about the UI for this and had proposed that any "extra" functionality that the consumer needs to add per legend item would be housed in an overflow context menu. Essentially:

cchaos · 2019-07-02T12:43:55Z

Also I'm not sure if this was introduced in this branch or not but I saw a bug in the Legend / hide legend items by series example when clicking to hide the series:

markov00 · 2019-07-02T13:02:10Z

Hide/show series (currently the only interaction on TSVB, but not an interaction on vizlib)

Is this not the current interaction by clicking on the legend item?

Clicking on vislib chart just only open the contextual menu with the color picker and the + - buttons to filter the element, where in tsvb clicking doesn't add any filter, just hide/show the series.

So the click will just hide/show the series without adding any filter. On a different PR we can add the contextual menu back again, so you can add any type of action on the selected legend item.

markov00 · 2019-07-03T17:41:38Z

@cchaos I've updated the code here with the icon when hiding the series

nickofthyme

LGTM, Tested locally.

cchaos · 2019-07-08T15:21:32Z

src/components/legend/legend_item.tsx

@@ -46,29 +46,32 @@ class LegendItemComponent extends React.Component<LegendItemProps, LegendItemSta
    const { legendItemKey } = this.props;
    const { color, label, isSeriesVisible, displayValue, onMouseEnter, onMouseLeave } = this.props;

-    const onTitleClick = this.onLegendTitleClick(legendItemKey);
+    const onTitleClick = this.onVisibilityClick(legendItemKey);

    const showLegendDisplayValue = this.props.chartStore!.showLegendDisplayValue.get();
    const isSelected = legendItemKey === this.props.chartStore!.selectedLegendItemKey.get();
    const hasDisplayValue = this.props.chartStore!.showLegendDisplayValue.get();
    const hasTitleClickListener = Boolean(this.props.chartStore!.onLegendItemClickListener);
    return (
      <div className="echLegendItem" onMouseEnter={onMouseEnter} onMouseLeave={onMouseLeave}>


Can you append a class of echLegendItem-isHidden if it's hidden and then in the CSS do
.echLegendItem-isHidden { color: $euiColorDarkShade }

done in 42adb3b

cchaos · 2019-07-08T15:22:22Z

src/components/legend/legend_item.tsx

+    const iconType = isSeriesVisible ? 'dot' : 'eyeClosed';
+    const iconColor = isSeriesVisible ? color : undefined;
+    const ariaLabel = isSeriesVisible ? 'series color' : 'series hidden';
+    const viewBox = isSeriesVisible ? undefined : '-3 -3 22 22';


Interesting way of sizing down the SVG, does this work in all browsers?

yes, or at least it works correctly on Chrome, Firefox and IE 11. viewBox is on the svg specs supported since 1.1

cchaos · 2019-07-08T15:24:17Z

src/components/legend/legend_item.tsx

+    // TODO add color picker
+    const iconType = isSeriesVisible ? 'dot' : 'eyeClosed';
+    const iconColor = isSeriesVisible ? color : undefined;
+    const ariaLabel = isSeriesVisible ? 'series color' : 'series hidden';


We have learned that aria-label attributes directly on svg's don't work with screen readers. You should apply this to the wrapping div. I would also add this as the title attribute to the wrapping div.

done in e40a42d

cchaos

LGTM. Thanks!

# [8.0.0](v7.2.1...v8.0.0) (2019-07-15) ### Code Refactoring * **legend:** remove visibility button ([#252](#252)) ([90a1ba7](90a1ba7)) ### Features * **style:** allow fill and stroke overrides ([#258](#258)) ([99c5e9f](99c5e9f)) ### BREAKING CHANGES * **style:** `LineStyle`, `AreaStyle` and `BarSeriesStyle` types differs on the optional values. `stroke` and `fill` on the theme or specific series style now override the computed series color. * fix: no unused locals error on theme * fix: avoid key prop override by spread operator * test: increase test coverage on PR changes * fix: fontSize is now always a number * test: increase coverage for rendendering * refactor(story): simplify theme merging on `with value label` story * refactor: removed mergeWithDefaultTheme * **legend:** the `onLegendItemClick` click handler is no longer applied when clicking on the title. Instead a simple visibility change is applied.

markov00 · 2019-07-15T10:16:25Z

🎉 This PR is included in version 8.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [8.0.0](elastic/elastic-charts@v7.2.1...v8.0.0) (2019-07-15) ### Code Refactoring * **legend:** remove visibility button ([opensearch-project#252](elastic/elastic-charts#252)) ([efb3823](elastic/elastic-charts@efb3823)) ### Features * **style:** allow fill and stroke overrides ([opensearch-project#258](elastic/elastic-charts#258)) ([1648996](elastic/elastic-charts@1648996)) ### BREAKING CHANGES * **style:** `LineStyle`, `AreaStyle` and `BarSeriesStyle` types differs on the optional values. `stroke` and `fill` on the theme or specific series style now override the computed series color. * fix: no unused locals error on theme * fix: avoid key prop override by spread operator * test: increase test coverage on PR changes * fix: fontSize is now always a number * test: increase coverage for rendendering * refactor(story): simplify theme merging on `with value label` story * refactor: removed mergeWithDefaultTheme * **legend:** the `onLegendItemClick` click handler is no longer applied when clicking on the title. Instead a simple visibility change is applied.

style: add icon for hidden series

0a94cc0

markov00 requested review from emmacunningham and nickofthyme July 3, 2019 17:42

markov00 added :legend Legend related issue :styling Styling related issue enhancement New feature or request labels Jul 3, 2019

nickofthyme approved these changes Jul 8, 2019

View reviewed changes

cchaos suggested changes Jul 8, 2019

View reviewed changes

markov00 added 2 commits July 10, 2019 13:01

style: add isHidden class on legend item

42adb3b

style(legend): move aria-label to div

e40a42d

cchaos approved these changes Jul 10, 2019

View reviewed changes

markov00 merged commit 90a1ba7 into elastic:master Jul 15, 2019

markov00 added the released Issue released publicly label Jul 15, 2019

markov00 deleted the remove_visibility_button branch November 25, 2020 11:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(legend): remove visibility button #252

refactor(legend): remove visibility button #252

markov00 commented Jul 2, 2019 •

edited

Loading

codecov-io commented Jul 2, 2019 •

edited

Loading

cchaos commented Jul 2, 2019

cchaos commented Jul 2, 2019

markov00 commented Jul 2, 2019

markov00 commented Jul 3, 2019

nickofthyme left a comment

cchaos Jul 8, 2019

markov00 Jul 10, 2019

cchaos Jul 8, 2019

markov00 Jul 10, 2019

cchaos Jul 8, 2019

markov00 Jul 10, 2019

cchaos left a comment

markov00 commented Jul 15, 2019

refactor(legend): remove visibility button #252

refactor(legend): remove visibility button #252

Conversation

markov00 commented Jul 2, 2019 • edited Loading

Summary

Checklist

codecov-io commented Jul 2, 2019 • edited Loading

Codecov Report

cchaos commented Jul 2, 2019

cchaos commented Jul 2, 2019

markov00 commented Jul 2, 2019

markov00 commented Jul 3, 2019

nickofthyme left a comment

Choose a reason for hiding this comment

cchaos Jul 8, 2019

Choose a reason for hiding this comment

markov00 Jul 10, 2019

Choose a reason for hiding this comment

cchaos Jul 8, 2019

Choose a reason for hiding this comment

markov00 Jul 10, 2019

Choose a reason for hiding this comment

cchaos Jul 8, 2019

Choose a reason for hiding this comment

markov00 Jul 10, 2019

Choose a reason for hiding this comment

cchaos left a comment

Choose a reason for hiding this comment

markov00 commented Jul 15, 2019

markov00 commented Jul 2, 2019 •

edited

Loading

codecov-io commented Jul 2, 2019 •

edited

Loading