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

feat(legend): display series value (dependent on hover) & sort in legend #155

Merged
merged 19 commits into from
Apr 10, 2019

Conversation

emmacunningham
Copy link
Contributor

@emmacunningham emmacunningham commented Apr 6, 2019

Summary

close #143
close #133

This PR adds the feature to display a value next to a legend item, where the value is either the current value of the crosshair tooltip or, if there is no corresponding tooltip value the last value in the series.

legend_hover

A prop on the Settings spec showLegendDisplayValue (default true) will toggle the appearence of the value. The screencap below also shows how the additional value displays for each legend Position option.

legend_position

The legend items are sorted by a specified sortIndex on the SeriesSpec (default undefined, where undefined items will appear last). We will separately add the feature to enable subseries sorting by allowing the user to specify a sortIndexAccessor (see this comment).

legend_sort

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

  • 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

@emmacunningham emmacunningham added enhancement New feature or request wip work in progress :legend Legend related issue labels Apr 6, 2019
@codecov-io
Copy link

codecov-io commented Apr 6, 2019

Codecov Report

Merging #155 into master will increase coverage by 0.21%.
The diff coverage is 97.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #155      +/-   ##
==========================================
+ Coverage   94.06%   94.28%   +0.21%     
==========================================
  Files          34       34              
  Lines        1753     1801      +48     
  Branches      223      228       +5     
==========================================
+ Hits         1649     1698      +49     
  Misses         90       90              
+ Partials       14       13       -1
Impacted Files Coverage Δ
src/lib/series/specs.ts 100% <ø> (ø) ⬆️
src/specs/settings.tsx 26.47% <0%> (-0.81%) ⬇️
src/lib/series/series.ts 95.03% <100%> (+0.5%) ⬆️
src/state/chart_state.ts 96.21% <100%> (+0.38%) ⬆️
src/lib/utils/interactions.ts 100% <100%> (ø) ⬆️
src/lib/series/tooltip.ts 75.86% <100%> (+12.7%) ⬆️
src/lib/series/legend.ts 100% <100%> (ø) ⬆️
src/state/annotation_utils.ts 100% <0%> (ø) ⬆️
src/lib/series/domains/x_domain.ts 100% <0%> (ø) ⬆️
... and 7 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 3ab9985...c87f7c4. Read the comment docs.

@emmacunningham
Copy link
Contributor Author

@markov00 Can address settings.tsx tests in a different PR since that will have quite a bit of things unrelated to this PR (currently no tests for that & it is the one with the lowest coverage).

@emmacunningham emmacunningham requested a review from markov00 April 8, 2019 02:13
@emmacunningham emmacunningham removed the wip work in progress label Apr 8, 2019
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

@emmacunningham I'm not sure sorting by the last bucket is the right way to do it because sorting happens on ES side and can depends on a different aggregation value. In the following snapshots you can see that with the same aggregation, I can sort by count of elements in the bucket or by average bytes.

Screenshot 2019-04-08 at 10 26 18

Screenshot 2019-04-08 at 10 26 05

I think we can propose something like adding an index prop to each series/subseries to enable sorting and let ES decide the right order on its values keeping also the ascending/descending property out our our charts.

if will be awesome if we can add in the future (not needed on this PR) also an index for each element, because happens that when sorting on es, each bucket can be sorted differently, in that case something like an indexAccessor can be a good way to implement (that is the element index on the array by default).

@emmacunningham emmacunningham requested a review from markov00 April 9, 2019 19:46
@emmacunningham
Copy link
Contributor Author

@markov00 Have removed the props to specify ascendin or descending and instead added a series spec sortIndex which allows the user to assign an indexical value which we will sort the series by. Also updated the example in the story with sample TVSB data to demonstrate how this would work with that dataset (editable array which is used to set the sortIndex props on each series).

@markov00 markov00 mentioned this pull request Apr 9, 2019
93 tasks
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested locally and works fine expect two minor issue to notice:

If you hide a series from the legend, the series value is no longer updated. I think a better UX should be to hide also the last bucket value if the series is hidden on the legend

hide-series-hide-legend-value

This other issue is marginal and can be resolved when the design team will take care a bit of how we can design this legend. on horizontal legend the legend value is pushed to the end of the line. This is caused by the use of flexbox for making a grid like that.
Screenshot 2019-04-10 at 10 20 23

@emmacunningham
Copy link
Contributor Author

@markov00 have updated the legend element so that when the series is not visible, the display value also is hidden

legend_hide_display

will add the horizontal legend position issue to the issue tracking legend styling!

@emmacunningham emmacunningham merged commit 78af858 into elastic:master Apr 10, 2019
markov00 pushed a commit that referenced this pull request Apr 10, 2019
# [3.9.0](v3.8.0...v3.9.0) (2019-04-10)

### Features

* **legend:** display series value (dependent on hover) & sort in legend ([#155](#155)) ([78af858](78af858))
@markov00
Copy link
Member

🎉 This PR is included in version 3.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Apr 10, 2019
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [3.9.0](elastic/elastic-charts@v3.8.0...v3.9.0) (2019-04-10)

### Features

* **legend:** display series value (dependent on hover) & sort in legend ([opensearch-project#155](elastic/elastic-charts#155)) ([ebdb000](elastic/elastic-charts@ebdb000))
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
Projects
None yet
3 participants