-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Lens] New value labels config option for bar charts #81776
Conversation
Pinging @elastic/kibana-app (Team:KibanaApp) |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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.
Stacked charts are not supported yet (it requires some more design and features from the charting lib)
I know we made this decision but can you refresh my mind why that's the case? It looks like the values would like quite nice on stacked bars as well because they are only shown within the bar.
Histogram charts (time and number) are not supported yet
I probably missed that part - why can't we use this setting for histogram charts again?
}), | ||
histogram: i18n.translate('xpack.lens.xyChart.valuesDisabledHelpText', { | ||
defaultMessage: | ||
'This setting cannot be applied to bar charts having time dimension on the x axis.', |
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.
This also applies to number histograms on the x axis, right?
@@ -386,6 +425,19 @@ export function XYChart({ | |||
}; | |||
return style; | |||
}; | |||
// Based on the XY toPreviewExpression logic |
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.
Let's move this logic to the toPreviewExpression
function itself:
...state.legend, |
Simply overwrite the state setting of show value labels with hide
in there, then you don't need to detect a preview expression in here.
} | ||
: { horizontal: HorizontalAlignment.Center }, | ||
offsetX: isHorizontal ? 10 : 0, | ||
offsetY: isHorizontal ? 0 : -5, |
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.
This might be a personal feeling, but IMHO the labels are too close to the upper edge of the bar - can we go to -10 or something here?
…a into feature/lens/value_labels_2
@dej611 I definitely see your point. So to rephrase (just to make sure I got it): For splitted bars and histogram charts "broken" value labels are very common with the default configuration (very likely for a user to run into such a situation). While it's also possible to create unstacked bars with really weird labels, those happen less often so it's the right trade-off to roll out this feature just for the cases that work most of the time, and wait till elastic-charts has a better handling for value labels in general until rolling out the feature there as well. (for reference, this is an unstacked bar chart with weird partial label placement) |
@@ -73,6 +85,39 @@ const legendOptions: Array<{ id: string; value: 'auto' | 'show' | 'hide'; label: | |||
}, | |||
]; | |||
|
|||
// TODO: inside/outside logic requires some more work on the elastic-chart side |
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.
Nit: Let's track that outside of the code.
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.
Added some small comments, but that's looking pretty good, great work!
@@ -402,6 +409,7 @@ export interface XYArgs { | |||
export interface XYState { | |||
preferredSeriesType: SeriesType; | |||
legend: LegendConfig; | |||
valueLabels: ValueLabelConfig; |
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.
As this is not optional, it changes the current state shape and we require a migration to make it work (existing saved objects won't have the valueLabels
property. Let's either make it optional (and default the behavior to hide
) or add the migration using the regular migration framework
// This format double fixes two issues in elastic-chart | ||
// * when rotating the chart, the formatter is not correctly picked | ||
// * in some scenarios value labels are not strings, and this breaks the elastic-chart lib | ||
valueFormatter: (d: unknown) => yAxis?.formatter?.convert(d) || '', |
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.
Is this an elastic-charts bug? If yes we should open an issue for it. Also, it seems like this behavior is complex enough to cover it with a unit test.
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.
The latter bug (non-string value handling) has been reported already. The former needs to be reported.
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.
Gotcha, thanks for confirming. LGTM if it's tested and reported.
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.
Did some testing, but not complete code review
...theme.barSeriesStyle, | ||
displayValue: { | ||
fontSize: { min: VALUE_LABELS_MIN_FONTSIZE, max: VALUE_LABELS_MAX_FONTSIZE }, | ||
fill: { textInverted: true, textBorder: true }, |
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'm honestly finding the text to be more readable without the text border. I know we thought it was important earlier, but I'm rethinking this. Want to try without it?
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 ran some tests here with current styling, no border or increased border size.
Current border
Thicker border
No border
Personally I find the thicker version easier to read compared to both current state and no border at all (exp. with bars with lower contrast with the text). But I see no big readability gain for few bars or vertical charts with many bars.
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.
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.
No need for a PR on the elastic-chart
side.
The textBorder
already accepts a number
to increase the size of it. Will update this PR right now.
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 was mostly suggesting it because I think having good defaults is important, and this looks like the kind of thing we would want to set by default for elastic-charts.
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'll open an issue on the repo
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.
elastic-chart issue: elastic/elastic-charts#886
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.
There are other considerations we should consider before baking this into Elastic Charts. I've commented separately on that issue.
})} | ||
condition={!hasNonBarSeries} | ||
tooltipContent={tooltipContentValueLabels} | ||
condition={!hasNonBarSeries && (!hasBarNotStacked || isHistogramSeries)} |
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 was honestly a bit confused by this as well, and it seems like @flash1293 was too. The problem is that we now have completely opposite settings. The "fill" settings are only for Line charts, and the "value labels" settings are only for bar charts. Because of this, I don't think we should ever disable the entire menu- I would like to always enable the menu, with help text on each individual setting. What do you think?
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 think that's a good idea, @wylieconlon - seems like the current logic is too smart to stay easily understandable.
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.
Updated PR
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.
Functionality wise this looks good to me (just left one nit), but I think we have to work on the labels:
Active state:
Display: Show/Hide seems confusing to me, what does "hiding" a "display" mean? - maybe "Labels"? maybe "Labels on chart"?
Not applicable because of histogram x axis:
Two problems with this label:
- It's also shown when looking at a line chart (like in the screenshot), making the wording a bad fit
- It's not only about time, it's also about number histograms
Suggestion: "This setting can only be used on categorical, un-stacked bar charts"
Not applicable because of chart type:
"Try other chart types" leaves the user guessing - where can they use it? Also the grammar seems weird (but take it with a grain of salt, my english skills are pretty bad)
Suggestion (same as for the other error case, simplifying the UI here): "This setting can only be used on categorical, un-stacked bar charts"
Feedback from native speakers appreciated (especially @gchaps)
|
||
const baseThemeWithMaybeValueLabels = !shouldShowValueLabels | ||
? chartTheme | ||
: mergeThemeWithValueLabelsStyling(chartTheme, valueLabels || 'hide', shouldRotate); |
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.
nit: valueLabels
should always be set, right?
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 think so. I was thinking about backward compatibility here, but probably the expression evaluation should ensure that the value is set.
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.
Yep, expressions are regenerated every time, so it's safe to assume it's available here because of the default value in to_expression
Updated the messages as you suggested @flash1293 . |
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.
Tested and LGTM. Left one UI nit
}) => { | ||
const [open, setOpen] = useState(false); | ||
|
||
const iconType: string | IconType = typeof type === 'string' ? typeToIconMap[type] : type; | ||
const panelClassName = larger ? 'lnsXyToolbar__popoverLarger' : 'lnsXyToolbar__popover'; |
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.
Not sure about this workaround - maybe @MichaelMarcialis has an idea how to improve this.
For context: To show why an option in the popover is disabled, a tooltip icon is added, but it's slightly too large to fit into the same row for the standard width of these popovers, so it has to get enlarged:
The only thing I can think of is to put the tooltip on the disabled form element itself instead of an additional icon, then the popover is wide enough and we don't need a separate class. The downside is the missing affordance - does the user know they have to hover the form element to get a reason for it being disabled?
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.
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.
The only thing I can think of is to put the tooltip on the disabled form element itself instead of an additional icon, then the popover is wide enough and we don't need a separate class. The downside is the missing affordance - does the user know they have to hover the form element to get a reason for it being disabled?
Yes, this was the reason of the conditional icon state. I'll update the PR with a better alignment 👍
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.
Hey, all. If I'm understanding correctly, and the "missing values" field is one that will always be disabled when any visualization other than line and area are selected, my first instinct would be to conditionally remove that form field altogether in those instances. I'm not a fan of showing users disabled form fields where users have little-to-no ability to re-enable (assuming the currently selected visualization type is the desired one).
For example, if I know I want a bar chart for my visualization, why even present me with a field that I'll never be able to interact with?
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.
Updated PR to hide the field when the chart is not of type area or line.
Also improved icon alignment as in the picture above for Series color
.
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.
Code review only.
Left a comment regarding the text treatment and I see that @MichaelMarcialis was involved for a UX consideration. Let me know if other specific feedback is needed.
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.
This is looking great! I've left two small comments/questions for your review.
Additionally, I was thinking about the current EuiIcon
usage, shown in the screenshot below. That icon makes me think of a text box tool, rather than visualization value options. Would something like the number
icon in EUI be a better fit?
<EuiFormRow | ||
display="columnCompressed" | ||
label={ | ||
<TooltipWrapper | ||
tooltipContent={i18n.translate( | ||
'xpack.lens.xyChart.valuesStackedDisabledHelpText', | ||
{ | ||
defaultMessage: | ||
'This setting can only be used on categorical, un-stacked bar charts', | ||
} | ||
)} | ||
condition={isValueLabelsDisabled} | ||
> | ||
<span> | ||
{i18n.translate('xpack.lens.shared.chartValueLabelVisibilityLabel', { | ||
defaultMessage: 'Labels', | ||
})}{' '} | ||
{isValueLabelsDisabled ? ( | ||
<EuiIcon | ||
type="questionInCircle" | ||
color="subdued" | ||
size="s" | ||
className="eui-alignTop" | ||
/> | ||
) : null} | ||
</span> | ||
</TooltipWrapper> | ||
} | ||
> | ||
<EuiButtonGroup | ||
isFullWidth | ||
legend={i18n.translate('xpack.lens.shared.chartValueLabelVisibilityLabel', { | ||
defaultMessage: 'Labels', | ||
})} | ||
isDisabled={isValueLabelsDisabled} | ||
data-test-subj="lnsValueLabelsDisplay" | ||
name="valueLabelsDisplay" | ||
buttonSize="compressed" | ||
options={valueLabelsOptions} | ||
idSelected={ | ||
valueLabelsOptions.find(({ value }) => value === valueLabelsVisibilityMode)!.id | ||
} | ||
onChange={(modeId) => { | ||
const newMode = valueLabelsOptions.find(({ id }) => id === modeId)!.value; | ||
setState({ ...state, valueLabels: newMode }); | ||
}} | ||
/> | ||
</EuiFormRow> |
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.
@dej611, @flash1293: In relation to my previous comment, would it also make sense to only show this "Labels" form field when a user has selected a vertical or horizontal bar visualization (given the current limitations)? Again, doing so would prevent showing the user a disabled form field that they would be unable to re-enable, if they were to select a non-bar type visualization.
In the situation that all form fields within this popover would be hidden (for example, if a stacked bar visualization was selected), then I would suggest we then hide the parent "Values" button that the popover emanates from.
Thoughts?
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.
It sounds we're likely back to square one here. :D
There's a plan to make value labels visible for all types of bars in the future: the way we handle this transition period (until the charting library supports more options or there's a decision to enable them as is) it's taking very long time to settle to a single UI, I think.
I do not have strong opinion here: it started with the button disabled and then enabled by request, so I do not mind either way.
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 think we struggled with the whole hiding vs. disabling in a bunch of other places as well. What do y'all think about merging this PR to avoid ongoing maintenance costs, come to a conclusion for Lens in general, then making everything consistent in a separate PR? This is how we handled lots of ongoing changes in the UI previously a few times.
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.
@flash1293: I'm fine with whatever approach you all deem best (either in this PR or a separate PR in the future). To be clear though, my concern is not about consistency.
@dej611: If the plan is to make the value labels option available for all bar-type visualizations in the future, then I would be fine with this form field being present whenever any bar-type visualization is selected, and simply disabling for now if it happens to be a stacked bar or histogram.
That said, what I would continue to find problematic is that this form field currently shows even when a non-bar visualization is selected. For example, this option continues to show for me (in a disabled state) when I've selected line, area, etc. Since a user can never interact with it for those non-bar visualization types, could we not hide it?
'xpack.lens.xyChart.valuesStackedDisabledHelpText', | ||
{ | ||
defaultMessage: | ||
'This setting can only be used on categorical, un-stacked bar charts', |
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.
Assuming my previous suggestion to conditionally hide this field is applied, we'll need to update this tooltip. Additionally, as a novice Lens user, it took me a few moments to understand what I had to do to my bar chart in order to be allowed to enable this setting. I'm wondering if verbiage such as this might make it clearer: This setting may be modified on categorical bar charts, but not histograms.
Someone else could likely better wordsmith this, but I think the notice that this is not for histogram-oriented bar charts makes it a bit more obvious. Thoughts?
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.
Agree on the wording problem, but perhaps it could be addressed in a separate PR?
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'm fine if you would prefer to update this text in a separate PR.
@MichaelMarcialis @dej611 To summarize:
Sounds good to me. |
|
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.
Thanks so much for making these changes, @dej611! Approving.
"Categorical, un-stacked bar charts" is a little hard to parse. How about: This setting only applies to categorical bar charts that are not stacked. If the control is greyed out, would it be ok to shorten these two lines: This setting may be modified on area and stacked area charts, but not percentage area charts. To This setting cannot be changed on percentage area charts. |
barSeriesStyle: { | ||
...theme.barSeriesStyle, | ||
displayValue: { | ||
fontSize: { min: VALUE_LABELS_MIN_FONTSIZE, max: VALUE_LABELS_MAX_FONTSIZE }, | ||
fill: { textInverted: true, textBorder: 2 }, | ||
alignment: isHorizontal | ||
? { | ||
vertical: VerticalAlignment.Middle, | ||
} | ||
: { horizontal: HorizontalAlignment.Center }, | ||
offsetX: isHorizontal ? VALUE_LABELS_HORIZONTAL_OFFSET : 0, | ||
offsetY: isHorizontal ? 0 : VALUE_LABELS_VERTICAL_OFFSET, | ||
}, | ||
}, | ||
}, |
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.
As described here elastic/elastic-charts#886 (comment) we should move these changes into the EUI chart theme.
It can also be done after merging this PR.
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]async chunks size
History
To update your PR or re-run it, just comment with: |
Co-authored-by: Kibana Machine <[email protected]>
…82849) Co-authored-by: Kibana Machine <[email protected]>
Summary
This PR contains all the logic for the new bar chart value labels feature based on previous PoC feedback.
Fixed #69100
This is a minimal set of feature, specifically scoped to address only bar charts (
bar
andbar_horizontal
) with the following limits:hide
/show
) right nowTask todo list:
hide
by defaultChecklist
Delete any items that are not applicable to this PR.