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 all 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 @@ -11,7 +11,7 @@ import { EuiIconLegend } from '../assets/legend';

const typeToIconMap: { [type: string]: string | IconType } = {
legend: EuiIconLegend as IconType,
values: 'visText',
values: 'number',
};

export interface ToolbarPopoverProps {
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 @@ -256,6 +256,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 @@ -1867,6 +1868,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 @@ -1952,6 +1954,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 @@ -2023,6 +2026,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 @@ -131,6 +135,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 @@ -214,6 +223,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: 2 },
alignment: isHorizontal
? {
vertical: VerticalAlignment.Middle,
}
: { horizontal: HorizontalAlignment.Center },
offsetX: isHorizontal ? VALUE_LABELS_HORIZONTAL_OFFSET : 0,
offsetY: isHorizontal ? 0 : VALUE_LABELS_VERTICAL_OFFSET,
},
},
},
Comment on lines +242 to +256
Copy link
Member

@markov00 markov00 Nov 6, 2020

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.

};
}

function getIconForSeriesType(seriesType: SeriesType): IconType {
return visualizationTypes.find((c) => c.id === seriesType)!.icon || 'empty';
}
Expand Down Expand Up @@ -254,7 +297,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 @@ -396,6 +439,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, shouldRotate);

const colorAssignments = getColorAssignments(args.layers, data, formatFactory);

return (
Expand All @@ -408,7 +461,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 @@ -613,6 +666,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 Down Expand Up @@ -649,9 +706,7 @@ export function XYChart({
palette.params
);
},
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 @@ -723,7 +778,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