-
Notifications
You must be signed in to change notification settings - Fork 121
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: add option to toggle value labels on bar charts #182
feat: add option to toggle value labels on bar charts #182
Conversation
Codecov Report
@@ Coverage Diff @@
## master #182 +/- ##
==========================================
+ Coverage 97% 97.19% +0.19%
==========================================
Files 35 35
Lines 1870 1962 +92
Branches 249 285 +36
==========================================
+ Hits 1814 1907 +93
+ Misses 49 48 -1
Partials 7 7
Continue to review full report at Codecov.
|
We can maybe have a small subset of checking cases, like:
|
@markov00 Added the toggling between alternating display values and repositioning for the edge cases (when bar height is too small or text appearing above bar): |
1bc6100
to
5d97a7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked the functionality, not code for the moment and I'm seeing few things that we can improve before releasing this:
- we should let the text overflow the width of the bar, this because it's a rare case that we have wider bars and smaller numbers, but in most case we can have big numbers with smaller bar width. The overflow should work by default, and we can add a prop to disable overflow and wrap text to the bar width, but I presume this will be less used option.
- the wrapping happens, strangely also when changing the padding, and the way it works is a bit strange and unpredictable:
-
the padding should work in the same way for all labels, see attached gif: seems that 7.00 and 3.00 act in a different way but them are both below the bottom edge of the bar. The 0.15 label is fixed until a certain value and that suddently moves down and than up.
I think we should have a better predictable way to move labels, independently on their current positions, move all down or up a certain value (that we can call it something like offset instead of padding)
-
we should take care of rotations:
-
the
alternating value label
should be works the same on a rotated chart. If we always hide the first label on the left, we can can partially avoid having clipped labels on the first part of the chart. -
can you add some tests/story with an higher number of bars? to see clippings on edges
The implementation has been rewrittten to support the additional feedback/requests. Previously, we were drawing the labels directly in the bar chart elements, but that was causing issues where the text would be covered up by any neighboring bars due to react-konva z-indexing based on render order. Instead, now we create the labels on their own Layer which prevents the labels from getting covered up. When we add the TextAnnotation feature, this layer can be re-used to feed values into the TextAnnotation layer. Values will be positioned relative to chartRotation:
There is another prop that the user can set to have the value labels contained within the bar element itself (by default values will overflow): If this prop is set to true and the text height overflows the available bar space, no text is rendered: Also there is another prop which can be used to control if clipped values (overflowing the edges of the chart) should be hidden or visible (would like to address in a separate PR how to handle these values besides just hiding them): Something else (that I would like to address in a followup PR) is how to handle automatically adjusted values: currently if a bar height is not tall enough to hold the display value, we move the label to above the bar. This causes some confusion when also applying an offset; should the label also move (as it currently does)? If so, then the offset amount will be greater than for the other labels which fit inside of the bar. One strategy might be to only apply the offset once it exceeds the automatically adjusted text's already offset position. A select knob has been added to the story to change the data volume from small, medium, and high: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work Emma! I think it's a neat implementation.
Tested locally on Chrome and I can correctly see all the labels.
# [4.1.0](v4.0.2...v4.1.0) (2019-05-04) ### Features * add option to toggle value labels on bar charts ([#182](#182)) ([6d8ec0e](6d8ec0e))
🎉 This PR is included in version 4.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [4.1.0](elastic/elastic-charts@v4.0.2...v4.1.0) (2019-05-04) ### Features * add option to toggle value labels on bar charts ([opensearch-project#182](elastic/elastic-charts#182)) ([24784c3](elastic/elastic-charts@24784c3))
Summary
close #67
This PR adds the option to toggle value labels on chart elements. This does not handle edge cases (long display values overflowing their bars, etc.) and just implements these text labels directly on the BarGeometries. Future iterations of this feature may handle edge cases as well as implementing the text labels as
TextAnnotation
s.This PR adds a
showValueLabels
prop to the SeriesSpec such that it toggles the appearance of the value lables for a series.displayValue
has been added to the SeriesStyle such that it can be customized either from the theme or the series' custom style.One can defined the prop
alternatingDisplayValue=true
so that the value labels alternate every other element.If an element is not tall enough or the value label would overflow the top edge of the chart, we re-position the value label for that bar only (in the first case, we position the label above the bar; in the second, we position the label within the bar).
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.