feat(legend): add legend item actions by nickofthyme · Pull Request #749 · elastic/elastic-charts

nickofthyme · 2020-07-13T14:07:19Z

Summary

closes #717

Adds actions to legend items

New prop legendAction that takes a LegendAction type defined as...

export interface LegendActionProps {
  /**
   * Series id for the given series
   */
  series: SeriesIdentifier;
}
export type LegendAction = ComponentType<LegendActionProps>;

Vertical legend

Horizontal legend

Checklist

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

src/specs/settings.tsx

markov00 · 2020-07-13T15:45:20Z

src/components/legend/_legend_item.scss

    }
  }

+  &__main {


with this additional div, the height of the legend is less than the height of the echLegendItem.
The problem is that the series are highlighted when moving the mouse over the echLegendItem but the edit button appears only when the mouse is over the __main div

probably moving the 4px padding of the echLegendItem to the __main could solve the issue

I see what you are saying here but I'm having trouble with solution. You idea of putting the padding on the __main would work but the blue background would be extended to the added padding and applying it as a margin will not trigger the hover event.

The solution I came up with was to wrap the __main element in another element to apply the padding and use that elements hover state to trigger the action. Not ideal nesting divs but better than other ideas I think.

The series will still be highlighted on the chart when hovering over the additionalValue element but I think that look fine.

Alternatively, I was playing with moving the mouse event from the whole container to the label but this was not a great solution as it made things very clunky, and still didn't fully solve the issue.

I've tested that and I'm not convinced, I think we should ask @miukimiu what she thinks about that.
Few doubts I have:

I think we should highlight the whole text row, including the value on the left.

the visual feedback of the series highlight action (the blue highlight), coming from the mouse hovering an element on the legend, should be rendered visually whenever the mouse enters the active area

the highlight should be consistent and should not turn off and on when moving from a label the other (as it's currently doing)

the same highlight will be used with when using the keyboard to navigate the legend, probably also @myasonik has some ideas on how it's better to place this focus

In case, at the moment we can add the legend actions without thinking about this visual feedback, and we can reason about it when working on the keyboard navigation, what do you think?

I think the new design you're proposing much better calls out the ability to interact with it and simplifies the focus management quite a bit. I'm +1 on the new design.

That said, if y'all still really wanted the old design, you could do it with some :focus-within and + CSS operator trickiness but it would likely become a really brittle area of the code and wouldn't work the same in all browsers...

Ok I updated to the new design. See screenshot below

The final question I have is how to handle the chart hover over the element. Due to the padding between the text, there is a space between the legend items that would not be clickable but we are still highlighting the series in the chart (i.e. graying out all the other series).

Possible solutions

remove highlight until over hoverable area

keep as is, but when hovering below the action there will be no blue highlight and the series will be still be highlighted in the chart.

@markov00 updated pr with latest designs per zoom call. See 5b6f1a2

I think there still gaps between legend actions that create an epileptic effect, I can jump on a call to describe that

Let's fix this in a subsequent PR

stories/legend/11_legend_actions.tsx

src/specs/settings.tsx

codecov-commenter · 2020-07-16T03:57:33Z

Codecov Report

Merging #749 into master will increase coverage by 0.41%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master     #749      +/-   ##
==========================================
+ Coverage   74.48%   74.89%   +0.41%     
==========================================
  Files         267      282      +15     
  Lines        9147     9473     +326     
  Branches     1879     1929      +50     
==========================================
+ Hits         6813     7095     +282     
- Misses       2329     2368      +39     
- Partials        5       10       +5

Flag	Coverage Δ
#unittests	`74.46% <57.14%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
src/specs/settings.tsx	`86.20% <ø> (ø)`
src/components/legend/legend_item.tsx	`91.02% <50.00%> (-3.57%)`	⬇️
src/components/legend/legend.tsx	`100.00% <100.00%> (ø)`
src/chart_types/xy_chart/utils/specs.ts	`100.00% <0.00%> (ø)`
src/mocks/store/store.ts	`81.48% <0.00%> (ø)`
src/mocks/specs/specs.ts	`77.01% <0.00%> (ø)`
src/mocks/series/series_identifiers.ts	`100.00% <0.00%> (ø)`
src/mocks/series/utils.ts	`100.00% <0.00%> (ø)`
src/mocks/scale/scale.ts	`77.77% <0.00%> (ø)`
src/mocks/series/series.ts	`85.36% <0.00%> (ø)`
... and 10 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 daff503...a62e58b. Read the comment docs.

nickofthyme · 2020-07-16T19:21:10Z

IE 11 seems to be somewhat strange. Maybe the EUI components are not fully compatible in playground.

markov00

LGTM!

# [19.9.0](v19.8.1...v19.9.0) (2020-07-17) ### Features * **axis:** formatting different for label vs tooltip and legend ([#750](#750)) ([daff503](daff503)) * **legend:** add legend item actions and margins ([#749](#749)) ([8136dca](8136dca)), closes [#717](#717)

markov00 · 2020-07-17T18:18:23Z

🎉 This PR is included in version 19.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [19.9.0](elastic/elastic-charts@v19.8.1...v19.9.0) (2020-07-17) ### Features * **axis:** formatting different for label vs tooltip and legend ([#750](elastic/elastic-charts#750)) ([027a772](elastic/elastic-charts@027a772)) * **legend:** add legend item actions and margins ([opensearch-project#749](elastic/elastic-charts#749)) ([75ce7b0](elastic/elastic-charts@75ce7b0)), closes [opensearch-project#717](elastic/elastic-charts#717)

feat(legend): add legend item actions

2c3825a

nickofthyme added enhancement New feature or request :legend Legend related issue labels Jul 13, 2020

nickofthyme requested a review from markov00 July 13, 2020 14:07

markov00 reviewed Jul 13, 2020

View reviewed changes

nickofthyme added 6 commits July 13, 2020 17:47

fix: cleanup story, update prop naming, fix hover issues

8b303ea

fix: vrt delayed icon load

43bba16

test: update snapshots from latest changes

b2e77ef

docs: update api with latest changes

022ff68

refactor: remove action visibility toggle, add action to end

d598f57

refactor: only render action div when needed

b6b5fcc

nickofthyme added 2 commits July 15, 2020 23:20

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

d9d4c14

test: update screenshot with design changes

a07e23a

nickofthyme requested review from elizabetdev, markov00 and myasonik July 16, 2020 13:32

nickofthyme added 4 commits July 16, 2020 13:58

refactor: legend styles with latest design changes

5b6f1a2

feat: add legend margin to theme.legend styles

c6cc414

test: update screenshots with latest changes

a62e58b

docs: update the api from latest changes

99438bb

nickofthyme added 2 commits July 16, 2020 14:24

fix: unknown api tag

97d3529

fix: vrt issues, add skip and delay option to all.test.ts

cdace8d

markov00 approved these changes Jul 17, 2020

View reviewed changes

nickofthyme merged commit 8136dca into elastic:master Jul 17, 2020

nickofthyme deleted the legend-actions branch July 17, 2020 16:42

markov00 added the released Issue released publicly label Jul 17, 2020

nickofthyme mentioned this pull request Sep 15, 2020

fix: legend dark mode hover color #820

Merged

4 tasks

nickofthyme mentioned this pull request Jan 12, 2021

chore: legend gap removal #968

Merged

2 tasks

Conversation

nickofthyme commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Vertical legend

Horizontal legend

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nickofthyme commented Jul 16, 2020

Uh oh!

markov00 left a comment

Choose a reason for hiding this comment

Uh oh!

markov00 commented Jul 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nickofthyme commented Jul 13, 2020 •

edited

Loading

codecov-commenter commented Jul 16, 2020 •

edited

Loading