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

[Lens] New value labels config option for bar charts #81776

Merged
merged 28 commits into from
Nov 6, 2020
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
cb9777a
:sparkles: First pass with chart values
dej611 Oct 27, 2020
db0d21b
:sparkles: Almost completed implementation
dej611 Oct 27, 2020
b5c55bb
:bug: + :white_check_mark: Fix pending checks issues + add tests
dej611 Oct 28, 2020
95bfb64
:sparkles: Add histogram check + refactor of some shared logic/types
dej611 Oct 28, 2020
8bae6e6
:wrench: Increase minimum font size
dej611 Oct 29, 2020
6f97d07
:white_check_mark: Add more tests for value labels corner cases
dej611 Oct 30, 2020
8a3f7ff
Merge branch 'master' into feature/lens/value_labels_2
kibanamachine Oct 30, 2020
908160b
Merge branch 'master' into feature/lens/value_labels_2
kibanamachine Oct 30, 2020
9a1173a
:ok_hand: Incorporate review feedback
dej611 Oct 30, 2020
fe00a1b
Merge branch 'feature/lens/value_labels_2' of github.com:dej611/kiban…
dej611 Oct 30, 2020
75d9013
:recycle: Migrate to simpler expression
dej611 Oct 30, 2020
47b1046
:bug: Resolve backward compatibility
dej611 Nov 2, 2020
1119bf7
:lipstick: Dropdown always available with conditional disable logic f…
dej611 Nov 2, 2020
6fcf5ad
Merge branch 'master' into feature/lens/value_labels_2
kibanamachine Nov 2, 2020
a54028c
:label: Fix type issues
dej611 Nov 2, 2020
4a235c6
:white_check_mark: Fix tests with new UX/UI
dej611 Nov 2, 2020
5facb32
Merge branch 'feature/lens/value_labels_2' of github.com:dej611/kiban…
dej611 Nov 2, 2020
7ca4e34
:lipstick: Update value label border styling
dej611 Nov 2, 2020
25522d5
:globe_with_meridians: + :ok_hand: Revisited messages + remove unused…
dej611 Nov 3, 2020
f3d1ec1
:world_with_meridians: fix i18n issue
dej611 Nov 3, 2020
36a6c3e
:lipstick: Hide missing fields row on bar charts + better icon alignment
dej611 Nov 4, 2020
b46016c
Merge remote-tracking branch 'upstream/master' into feature/lens/valu…
dej611 Nov 4, 2020
b82da66
:fire: Remove old cruft
dej611 Nov 4, 2020
1f6f324
:white_check_mark: Align tests to new UX
dej611 Nov 4, 2020
b96c188
:label: Fix type issue
dej611 Nov 4, 2020
d9562aa
:ok_hand: Implemented review feedback
dej611 Nov 5, 2020
036bbde
:world_with_meridians: Change copy messages
dej611 Nov 6, 2020
6bac751
Merge branch 'master' into feature/lens/value_labels_2
kibanamachine Nov 6, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export interface ToolbarPopoverProps {
*/
groupPosition?: ToolbarButtonProps['groupPosition'];
buttonDataTestSubj?: string;
larger?: boolean;
}

export const ToolbarPopover: React.FunctionComponent<ToolbarPopoverProps> = ({
Expand All @@ -41,15 +42,17 @@ export const ToolbarPopover: React.FunctionComponent<ToolbarPopoverProps> = ({
isDisabled = false,
groupPosition,
buttonDataTestSubj,
larger,
}) => {
const [open, setOpen] = useState(false);

const iconType: string | IconType = typeof type === 'string' ? typeToIconMap[type] : type;
const panelClassName = larger ? 'lnsXyToolbar__popoverLarger' : 'lnsXyToolbar__popover';
Copy link
Contributor

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:
Screenshot 2020-11-03 at 15 39 26

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just realized we have something like this in lens already - it looks like this for the colors:
Screenshot 2020-11-03 at 16 54 56

No strong opinion which route to go but we should make sure to do it the same way in both places

Copy link
Contributor Author

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 👍

Copy link
Contributor

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?

Copy link
Contributor Author

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.


return (
<EuiFlexItem grow={false}>
<EuiPopover
panelClassName="lnsXyToolbar__popover"
panelClassName={panelClassName}
ownFocus
aria-label={title}
button={
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ const createArgsWithLayers = (layers: LayerArgs[] = [sampleLayer]): XYArgs => ({
isVisible: false,
position: Position.Top,
},
valueLabels: 'hide',
axisTitlesVisibilitySettings: {
type: 'lens_xy_axisTitlesVisibilityConfig',
x: true,
Expand Down Expand Up @@ -1794,6 +1795,7 @@ describe('xy_expression', () => {
yTitle: '',
yRightTitle: '',
legend: { type: 'lens_xy_legendConfig', isVisible: false, position: Position.Top },
valueLabels: 'hide',
tickLabelsVisibilitySettings: {
type: 'lens_xy_tickLabelsConfig',
x: true,
Expand Down Expand Up @@ -1876,6 +1878,7 @@ describe('xy_expression', () => {
yTitle: '',
yRightTitle: '',
legend: { type: 'lens_xy_legendConfig', isVisible: false, position: Position.Top },
valueLabels: 'hide',
tickLabelsVisibilitySettings: {
type: 'lens_xy_tickLabelsConfig',
x: true,
Expand Down Expand Up @@ -1945,6 +1948,7 @@ describe('xy_expression', () => {
yTitle: '',
yRightTitle: '',
legend: { type: 'lens_xy_legendConfig', isVisible: true, position: Position.Top },
valueLabels: 'hide',
tickLabelsVisibilitySettings: {
type: 'lens_xy_tickLabelsConfig',
x: true,
Expand Down
79 changes: 73 additions & 6 deletions x-pack/plugins/lens/public/xy_visualization/expression.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ import {
GeometryValue,
XYChartSeriesIdentifier,
StackMode,
RecursivePartial,
Theme,
VerticalAlignment,
HorizontalAlignment,
} from '@elastic/charts';
import { I18nProvider } from '@kbn/i18n/react';
import {
Expand Down Expand Up @@ -125,6 +129,11 @@ export const xyChart: ExpressionFunctionDefinition<
defaultMessage: 'Define how missing values are treated',
}),
},
valueLabels: {
flash1293 marked this conversation as resolved.
Show resolved Hide resolved
types: ['string'],
options: ['hide', 'inside'],
help: '',
},
tickLabelsVisibilitySettings: {
types: ['lens_xy_tickLabelsConfig'],
help: i18n.translate('xpack.lens.xyChart.tickLabelsSettings.help', {
Expand Down Expand Up @@ -206,6 +215,40 @@ export const getXyChartRenderer = (dependencies: {
},
});

function mergeThemeWithValueLabelsStyling(
theme: RecursivePartial<Theme>,
valuesLabelMode: string = 'hide',
isHorizontal: boolean
) {
const VALUE_LABELS_MAX_FONTSIZE = 15;
const VALUE_LABELS_MIN_FONTSIZE = 10;
const VALUE_LABELS_VERTICAL_OFFSET = -10;
const VALUE_LABELS_HORIZONTAL_OFFSET = 10;

if (valuesLabelMode === 'hide') {
return theme;
}
return {
...theme,
...{
barSeriesStyle: {
...theme.barSeriesStyle,
displayValue: {
fontSize: { min: VALUE_LABELS_MIN_FONTSIZE, max: VALUE_LABELS_MAX_FONTSIZE },
fill: { textInverted: true, textBorder: true },
Copy link
Contributor

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?

Copy link
Contributor Author

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

Screenshot 2020-11-02 at 12 33 27

Screenshot 2020-11-02 at 12 38 26

Screenshot 2020-11-02 at 12 47 10

Thicker border

Screenshot 2020-11-02 at 12 36 21

Screenshot 2020-11-02 at 12 37 07

Screenshot 2020-11-02 at 12 50 34

No border

Screenshot 2020-11-02 at 12 34 37

Screenshot 2020-11-02 at 12 37 42

Screenshot 2020-11-02 at 12 47 50

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dej611 Thanks for showing the options, I share your opinion that the thicker border is the easiest of these options. Will you submit a PR to elastic-charts for this? cc @markov00

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

alignment: isHorizontal
? {
vertical: VerticalAlignment.Middle,
}
: { horizontal: HorizontalAlignment.Center },
offsetX: isHorizontal ? VALUE_LABELS_HORIZONTAL_OFFSET : 0,
offsetY: isHorizontal ? 0 : VALUE_LABELS_VERTICAL_OFFSET,
},
},
},
};
}

function getIconForSeriesType(seriesType: SeriesType): IconType {
return visualizationTypes.find((c) => c.id === seriesType)!.icon || 'empty';
}
Expand Down Expand Up @@ -245,7 +288,7 @@ export function XYChart({
onClickValue,
onSelectRange,
}: XYChartRenderProps) {
const { legend, layers, fittingFunction, gridlinesVisibilitySettings } = args;
const { legend, layers, fittingFunction, gridlinesVisibilitySettings, valueLabels } = args;
const chartTheme = chartsThemeService.useChartsTheme();
const chartBaseTheme = chartsThemeService.useChartsBaseTheme();

Expand Down Expand Up @@ -387,6 +430,16 @@ export function XYChart({
return style;
};

const shouldShowValueLabels =
// No stacked bar charts
filteredLayers.every((layer) => !layer.seriesType.includes('stacked')) &&
// No histogram charts
!isHistogramViz;

const baseThemeWithMaybeValueLabels = !shouldShowValueLabels
? chartTheme
: mergeThemeWithValueLabelsStyling(chartTheme, valueLabels || 'hide', shouldRotate);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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


return (
<Chart>
<Settings
Expand All @@ -397,7 +450,7 @@ export function XYChart({
}
legendPosition={legend.position}
showLegendExtra={false}
theme={chartTheme}
theme={baseThemeWithMaybeValueLabels}
baseTheme={chartBaseTheme}
tooltip={{
headerFormatter: (d) => safeXAccessorLabelRenderer(d.value),
Expand Down Expand Up @@ -601,6 +654,10 @@ export function XYChart({
});
}

const yAxis = yAxesConfiguration.find((axisConfiguration) =>
axisConfiguration.series.find((currentSeries) => currentSeries.accessor === accessor)
);

const seriesProps: SeriesSpec = {
splitSeriesAccessors: splitAccessor ? [splitAccessor] : [],
stackAccessors: seriesType.includes('stacked') ? [xAccessor as string] : [],
Expand All @@ -611,9 +668,7 @@ export function XYChart({
xScaleType: xAccessor ? xScaleType : 'ordinal',
yScaleType,
color: () => getSeriesColor(layer, accessor),
groupId: yAxesConfiguration.find((axisConfiguration) =>
axisConfiguration.series.find((currentSeries) => currentSeries.accessor === accessor)
)?.groupId,
groupId: yAxis?.groupId,
enableHistogramMode:
isHistogram &&
(seriesType.includes('stacked') || !splitAccessor) &&
Expand Down Expand Up @@ -685,7 +740,19 @@ export function XYChart({
case 'bar_horizontal':
case 'bar_horizontal_stacked':
case 'bar_horizontal_percentage_stacked':
return <BarSeries key={index} {...seriesProps} />;
const valueLabelsSettings = {
displayValueSettings: {
// 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) || '',
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

showValueLabel: shouldShowValueLabels && valueLabels !== 'hide',
isAlternatingValueLabel: false,
isValueContainedInElement: true,
hideClippedValue: true,
},
};
return <BarSeries key={index} {...seriesProps} {...valueLabelsSettings} />;
case 'area_stacked':
case 'area_percentage_stacked':
return (
Expand Down
23 changes: 22 additions & 1 deletion x-pack/plugins/lens/public/xy_visualization/state_helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
*/

import { EuiIconType } from '@elastic/eui/src/components/icon/icon';
import { SeriesType, visualizationTypes, LayerConfig, YConfig } from './types';
import { FramePublicAPI } from '../types';
import { SeriesType, visualizationTypes, LayerConfig, YConfig, ValidLayer } from './types';

export function isHorizontalSeries(seriesType: SeriesType) {
return (
Expand Down Expand Up @@ -37,3 +38,23 @@ export const getSeriesColor = (layer: LayerConfig, accessor: string) => {
layer?.yConfig?.find((yConfig: YConfig) => yConfig.forAccessor === accessor)?.color || null
);
};

export function hasHistogramSeries(
layers: ValidLayer[] = [],
datasourceLayers?: FramePublicAPI['datasourceLayers']
) {
if (!datasourceLayers) {
return false;
}
const validLayers = layers.filter(({ accessors }) => accessors.length);

return validLayers.some(({ layerId, xAccessor }: ValidLayer) => {
const xAxisOperation = datasourceLayers[layerId].getOperationForColumnId(xAccessor);
return (
xAxisOperation &&
xAxisOperation.isBucketed &&
xAxisOperation.scale &&
xAxisOperation.scale !== 'ordinal'
);
});
}
Loading