[Lens] New value labels config option for bar charts #75426
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dej611
added
Team:Visualizations
dej611
changed the title
mbondyra
changed the title
|
@elasticmachine merge upstream |
|
@dej611 @flash1293 why aren't we using Elastic chart? https://elastic.github.io/elastic-charts/?path=/story/bar-chart--with-value-label seems like the capability already exists and supports also stacked bars ? I tried the pr twice, for some reason I don't see the feature |
wylieconlon
reviewed
Aug 20, 2020
There was a problem hiding this comment.
This is a really good start, and I'm glad you've started looking into improvements to the underlying elastic-charts implementation! I've left a lot of comments based on the implementations of "value labeling" that I've seen in other products, and if it's easier to go through them in a meeting we should.
The main changes that I was hoping for are:
- Providing the best possible defaults so that labels always look good
- Adding some additional styling options so that users have control
- Moving the labeling option to a top-level chart setting instead of per-dimension
- Keeping the form visible but disabled when the settings can't be used
x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx
Outdated
Show resolved
Hide resolved
| !chartHasMoreThanOneSeries && | ||
| shouldRotate && | ||
| yConfig && | ||
| yConfig?.find(({ forAccessor }) => forAccessor === accessor)?.showValueLabels; |
There was a problem hiding this comment.
I think the "show value labels" option should not be set on any layer, but on the chart as a whole.
There was a problem hiding this comment.
I can see both being valuable. @cchaos @AlonaNadler What do you think? Should "show values" be a dimension level, layer level or chart level setting? Or maybe offering multiple ways?
|
Since I don't think we've been working from a specific target, I think this might help. I'm expecting that with default settings for value labels, we'd get a visualization that looks like this:
There are a few parts of this that I want to call out specifically:
|
|
Integrated some of the feedback. thanks @wylieconlon and @AlonaNadler . |
wylieconlon
suggested changes
Aug 21, 2020
There was a problem hiding this comment.
Like all of the other features in Lens, we are trying to provide the best defaults with the least configuration, and that sometimes means that we won't offer full functionality. With that in mind, here's my feedback about how this is offered in the product:
- The "outside" labels don't work well in the current implementation because they are not rendered for the tallest bars. Unless this can be fixed inside elastic-charts, we should not offer this option.
- I found it almost impossible to see the value labels when dealing with histogram charts- date histogram in particular. I would prefer disabling it when using histogram mode.
- I originally thought that the multi-series charts would be hard to read, but they aren't as bad as I originally thought. This doesn't mean that the multi-series charts are easy to read, so maybe you should make a decision: is the risk of a bad chart rendering worth it for users?
- I was expecting that when I turn value labeling on, it would be turned on for all series. Instead, this PR has an intermediate state where I can have value labels on one Y axis but not another. This looks weird, and you should make it a global on/off setting:
|
Thanks @wylieconlon for your feedback, I think that is very useful.
That was a configuration glitch which can be partially solved.
Yes, I agree with that. Histograms increase the complexity of the chart already, and if combined with other option I see no particular values of value labels in there.
I may be leaning on the "yes" side here: enabling value labels is a precise action of the user already who can preview its result live.
Yes, the co-existence of both dimension-local and global setting has no sense, I agree. I am leaning on the global setting too, but heard some feedback about local that maybe is worth discuss ( @AlonaNadler , @flash1293 ). |
💔 Build Failed
Failed CI Steps
Test FailuresChrome X-Pack UI Functional Tests.x-pack/test/functional/apps/lens/dashboard·ts.lens app lens dashboard tests should be able to add filters/timerange by clicking in XYChartStandard OutStack TraceChrome X-Pack UI Functional Tests.x-pack/test/functional/apps/lens/dashboard·ts.lens app lens dashboard tests should be able to add filters/timerange by clicking in XYChartStandard OutStack TraceX-Pack Jest Tests.x-pack/plugins/lens/public/xy_visualization.xy_expression XYChart component it renders lineStandard OutStack Traceand 52 more failures, only showing the first 3. Build metricspage load bundle size
History
To update your PR or re-run it, just comment with: |
|
Hey Marco great job! 👏 Few comments:
questions from slack:
Im conflicted on that, I don't want the chart setting to become the long exhausting list of configurations. This setting makes sense, I wonder if the size and other configurations should exist also in the series. Personally I missed their existence
its a good start, in the future we should add labels to area and line charts
See my comment above, why? :)
you want to match to visualize for consistency or since you think it works better? if it's for consistency reasons - then its not important from my perspective. |
|
I've update the styling options prototype to include a new item in the toolbar for "Values". Here's a static preview:
I'm only concentrating on where the settings should live in the UI and not worrying about the Charts styling/handling of these values as I think most of that should be handled by the DataVis team. Mainly you can think of all the styling options for value labels to be a global-level chart setting, but users should be able to change the visibility per configuration. Some feedback for this PR:
|
|
@cchaos I agree with you about the font size setting. We are working with the charts team to do dynamic font sizing, my PR is only a draft for now but it'll give you an idea of where this is going: elastic/elastic-charts#789 |
|
Ideally:
Minimum
|
|
Dependencies: Automatic font size elastic/elastic-charts#788 |
10 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Based on the feedback received I've opened change the PR to enable various experiments on various directions with value labels:
The setting work globally per chart, but its configuration is reachable in different ways.
It is possible to set the size of the font for the value labels and their positioning
debounceoutsideoption and the longest barThe colors are currently hardcoded (
whitewheninsideandblackwhenoutside) but this should be improved hopefully with the contrasting PR inelastic-chartsThe value label setting is now enable on all the types of bar charts (stacked, multi-series, multi-layers, and break down)
elastic-charts.Note: there are definitely more options enabled now than what's needed, but I thought it may be worth exploring all of them.
Note 2: the Settings menu is going to be replaced and redesigned probably, so do not take it as final version.
Old content below
Show old version
#### First iteration features now removedOver timemode there's a glitch with 0-value bars, (already notified)[https://github.com/Zero values are printed on top of other values when in stacked histogram mode elastic-charts#790] in theelastic-chartsrepoOriginal post
This PR introduce value labels for the horizontal bar charts in Lens.
Now horizontal bar chart expose a new setting to enable value labels on the bars:
Here's a list of tasks to be completed before making this PR ready:
debugset of options be enabled to test different scenarios?Improve labels readability: as for now the
whitecolor is hardcoded, but need to be discussedelastic-chartslib directly with some extra options?Improve positioning of labels on the bar. It currently sits on the top right corner and its styling can be improved
elastic-chartslibDiscuss about hiding criterias: alternative (overlapping) values? Show only values that fit in the bar? What about small bars?
Possibly fix the value formatter issue for the
isValueContainedInElementoptionRevisit the i18n support labels
Ref issue: #69100
Checklist