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] Improved range formatter #80132

Merged
merged 36 commits into from
Oct 27, 2020
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
54ed36f
:sparkles: New alternative formatter for ranges
dej611 Sep 18, 2020
1904551
:white_check_mark: Add range formatter test for alternative format
dej611 Sep 18, 2020
080eb8a
Merge remote-tracking branch 'upstream/master' into feature/range-alt…
dej611 Sep 22, 2020
c14ff49
Merge remote-tracking branch 'upstream/master' into feature/range-alt…
dej611 Oct 12, 2020
004a580
:lipstick: Adopt new range format template
dej611 Oct 12, 2020
5a1224e
:lipstick: Revisit styling for range popover
dej611 Oct 14, 2020
fb15462
Merge remote-tracking branch 'upstream/master' into feature/range-alt…
dej611 Oct 14, 2020
a058a9c
:recycle: Rework the logic of the formatter to better handle the rang…
dej611 Oct 15, 2020
2db3014
:label: fix type issue
dej611 Oct 15, 2020
142cbfd
:lipstick: Use the arrow format rather than dash
dej611 Oct 15, 2020
8bae6d2
:lipstick: Restored append and prepend symbols
dej611 Oct 15, 2020
68b1f46
:lipstick: Picked better arrow symbol
dej611 Oct 15, 2020
6557b11
:lipstick: Add missing compressed attribute
dej611 Oct 15, 2020
f664fb0
:white_check_mark: Add more tests for range params scenarios
dej611 Oct 16, 2020
8d9547c
Merge branch 'master' into feature/range-alternative-formatter
kibanamachine Oct 16, 2020
fa51aa7
Update x-pack/plugins/lens/public/indexpattern_datasource/operations/…
dej611 Oct 16, 2020
89f82a8
Merge remote-tracking branch 'origin/master' into HEAD
wylieconlon Oct 16, 2020
268d82e
Merge branch 'feature/range-alternative-formatter' of github.com:dej6…
wylieconlon Oct 16, 2020
d1cf17b
Merge remote-tracking branch 'origin/master' into HEAD
wylieconlon Oct 16, 2020
0868abe
Merge branch 'master' into feature/range-alternative-formatter
kibanamachine Oct 19, 2020
7b860ff
:ok_hand: Refactor Expression phase based on review feedback
dej611 Oct 19, 2020
a6dff88
:bug: Fix default formatter handling
dej611 Oct 19, 2020
8395a50
:white_check_mark: Add test for default and override formatter scenarios
dej611 Oct 19, 2020
fd2ddfe
:label: Fix type check
dej611 Oct 19, 2020
6cf8a8b
:bug: Fix decimals inconsistency with range parameters in specific case
dej611 Oct 21, 2020
e265b95
pass through index pattern format unchanged if no Lens format is spec…
flash1293 Oct 21, 2020
b6ed4b8
Merge branch 'master' into feature/range-alternative-formatter
kibanamachine Oct 21, 2020
04b391a
:bug: Fix fieldFormatMap serialization issue
dej611 Oct 22, 2020
7a93683
:white_check_mark: Add fieldformatMap field to test edge cases
dej611 Oct 22, 2020
456a80b
:bug: Better handling to support already serialized formatters
dej611 Oct 22, 2020
6241055
Organize the formatting differently
wylieconlon Oct 22, 2020
f7fc98b
Merge pull request #2 from wylieconlon/ranges
dej611 Oct 26, 2020
45d0933
Merge branch 'master' into feature/range-alternative-formatter
kibanamachine Oct 26, 2020
6e24997
:wrench: Raise the limit for data plugin by 5kb
dej611 Oct 27, 2020
bcc2639
Merge branch 'master' into feature/range-alternative-formatter
dej611 Oct 27, 2020
adc0ff3
:recycle: Refactor to use async_services
dej611 Oct 27, 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 @@ -79,6 +79,33 @@ describe('getFormatWithAggs', () => {
expect(getFormat).toHaveBeenCalledTimes(1);
});

test('creates alternative format for range using the template parameter', () => {
const mapping = { id: 'range', params: { template: 'arrow_right' } };
const getFieldFormat = getFormatWithAggs(getFormat);
const format = getFieldFormat(mapping);

expect(format.convert({ gte: 1, lt: 20 })).toBe('1 → 20');
expect(getFormat).toHaveBeenCalledTimes(1);
});

test('handles Infinity values internally when no nestedFormatter is passed', () => {
const mapping = { id: 'range', params: { replaceInfinity: true } };
const getFieldFormat = getFormatWithAggs(getFormat);
const format = getFieldFormat(mapping);

expect(format.convert({ gte: -Infinity, lt: Infinity })).toBe('≥ −∞ and < +∞');
expect(getFormat).toHaveBeenCalledTimes(1);
});

test('lets Infinity values handling to nestedFormatter even when flag is on', () => {
const mapping = { id: 'range', params: { replaceInfinity: true, id: 'any' } };
const getFieldFormat = getFormatWithAggs(getFormat);
const format = getFieldFormat(mapping);

expect(format.convert({ gte: -Infinity, lt: Infinity })).toBe('≥ -Infinity and < Infinity');
expect(getFormat).toHaveBeenCalledTimes(1);
});

test('returns custom label for range if provided', () => {
const mapping = { id: 'range', params: {} };
const getFieldFormat = getFormatWithAggs(getFormat);
Expand Down
24 changes: 22 additions & 2 deletions src/plugins/data/common/search/aggs/utils/get_format_with_aggs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,35 @@ export function getFormatWithAggs(getFieldFormat: GetFieldFormat): GetFieldForma
id: nestedFormatter.id,
params: nestedFormatter.params,
});

const gte = '\u2265';
const lt = '\u003c';
let fromValue = format.convert(range.gte);
let toValue = format.convert(range.lt);
// In case of identity formatter and a specific flag, replace Infinity values by specific strings
if (params.replaceInfinity && nestedFormatter.id == null) {
const FROM_PLACEHOLDER = '\u2212\u221E';
const TO_PLACEHOLDER = '+\u221E';
fromValue = isFinite(range.gte) ? fromValue : FROM_PLACEHOLDER;
toValue = isFinite(range.lt) ? toValue : TO_PLACEHOLDER;
}

if (params.template === 'arrow_right') {
return i18n.translate('data.aggTypes.buckets.ranges.rangesFormatMessageArrowRight', {
defaultMessage: '{from} → {to}',
values: {
from: fromValue,
to: toValue,
},
});
}
return i18n.translate('data.aggTypes.buckets.ranges.rangesFormatMessage', {
defaultMessage: '{gte} {from} and {lt} {to}',
values: {
gte,
from: format.convert(range.gte),
from: fromValue,
lt,
to: format.convert(range.lt),
to: toValue,
},
});
});
Expand Down
42 changes: 38 additions & 4 deletions x-pack/plugins/lens/public/editor_frame_service/format_column.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,15 @@ interface FormatColumn {
format: string;
columnId: string;
decimals?: number;
nestedFormat?: string;
template?: string;
replaceInfinity?: boolean;
}

const supportedFormats: Record<string, { decimalsToPattern: (decimals?: number) => string }> = {
export const supportedFormats: Record<
string,
{ decimalsToPattern: (decimals?: number) => string }
> = {
number: {
decimalsToPattern: (decimals = 2) => {
if (decimals === 0) {
Expand Down Expand Up @@ -63,13 +69,27 @@ export const formatColumn: ExpressionFunctionDefinition<
types: ['number'],
help: '',
},
nestedFormat: {
types: ['string'],
help: '',
},
template: {
types: ['string'],
help: '',
},
replaceInfinity: {
types: ['boolean'],
help: '',
},
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 thinking a little bit further ahead with these arguments, and I would prefer that we make slightly more generic arguments instead of specifically for ranges. The main reason is that later in this release cycle I'm expecting to add the time scaling formatters, which might have some custom arguments.

So usually when we want complex arguments, we use one of two approaches:

  • Pass in a string of JSON which is parsed by the expression function
  • Create a one-off expression function to hold the specific argument

In this case I prefer passing a JSON string, because that's similar to the way we define formatters in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I was looking for a similar approach to avoid too many args, but couldn't find it in the codebase. Makes sense 👍 , I'll look now for that.

},
inputTypes: ['kibana_datatable'],
fn(input, { format, columnId, decimals }: FormatColumn) {
fn(input, { format, columnId, decimals, nestedFormat, template, replaceInfinity }: FormatColumn) {
return {
...input,
columns: input.columns.map((col) => {
if (col.id === columnId) {
const extraParams = { template, replaceInfinity };

if (supportedFormats[format]) {
return {
...col,
Expand All @@ -78,12 +98,26 @@ export const formatColumn: ExpressionFunctionDefinition<
params: { pattern: supportedFormats[format].decimalsToPattern(decimals) },
},
};
} else {
}
if (nestedFormat && supportedFormats[nestedFormat]) {
return {
...col,
formatHint: { id: format, params: {} },
formatHint: {
id: format,
params: {
id: nestedFormat,
params: {
pattern: supportedFormats[nestedFormat].decimalsToPattern(decimals),
},
...extraParams,
},
},
};
}
return {
...col,
formatHint: { id: format, params: { ...extraParams } },
};
}
return col;
}),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,8 @@ export function DimensionEditor(props: DimensionEditorProps) {
/>
)}

{selectedColumn && selectedColumn.dataType === 'number' ? (
{selectedColumn &&
(selectedColumn.dataType === 'number' || selectedColumn.operationType === 'range') ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to revisit this conditional logic later, it probably won't scale but it's fine for now.

<FormatSelector
selectedColumn={selectedColumn}
onChange={(newFormat) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,8 @@
@include euiFontSizeS;
min-height: $euiSizeXL;
width: 100%;
}

.lnsRangesOperation__popoverNumberField {
width: 100px;
dej611 marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ import {
EuiPopover,
EuiToolTip,
htmlIdGenerator,
keys,
} from '@elastic/eui';
import { keys } from '@elastic/eui';
import { IFieldFormat } from '../../../../../../../../src/plugins/data/common';
import { RangeTypeLens, isValidRange, isValidNumber } from './ranges';
import { FROM_PLACEHOLDER, TO_PLACEHOLDER, TYPING_DEBOUNCE_TIME } from './constants';
Expand All @@ -39,8 +39,8 @@ type LocalRangeType = RangeTypeLens & { id: string };
const getBetterLabel = (range: RangeTypeLens, formatter: IFieldFormat) =>
range.label ||
formatter.convert({
gte: isValidNumber(range.from) ? range.from : FROM_PLACEHOLDER,
lt: isValidNumber(range.to) ? range.to : TO_PLACEHOLDER,
gte: isValidNumber(range.from) ? range.from : -Infinity,
lt: isValidNumber(range.to) ? range.to : Infinity,
});

export const RangePopover = ({
Expand All @@ -55,7 +55,6 @@ export const RangePopover = ({
Button: React.FunctionComponent<{ onClick: MouseEventHandler }>;
isOpenByCreation: boolean;
setIsOpenByCreation: (open: boolean) => void;
formatter: IFieldFormat;
}) => {
const [isPopoverOpen, setIsPopoverOpen] = useState(false);
const [tempRange, setTempRange] = useState(range);
Expand Down Expand Up @@ -112,6 +111,7 @@ export const RangePopover = ({
<EuiFlexGroup gutterSize="s" responsive={false} alignItems="center">
<EuiFlexItem>
<EuiFieldNumber
className="lnsRangesOperation__popoverNumberField"
value={isValidNumber(from) ? Number(from) : ''}
onChange={({ target }) => {
const newRange = {
Expand All @@ -126,7 +126,6 @@ export const RangePopover = ({
<EuiText size="s">{lteAppendLabel}</EuiText>
</EuiToolTip>
}
fullWidth
compressed
placeholder={FROM_PLACEHOLDER}
isInvalid={!isValidRange(tempRange)}
Expand All @@ -137,6 +136,7 @@ export const RangePopover = ({
</EuiFlexItem>
<EuiFlexItem>
<EuiFieldNumber
className="lnsRangesOperation__popoverNumberField"
value={isValidNumber(to) ? Number(to) : ''}
onChange={({ target }) => {
const newRange = {
Expand All @@ -151,7 +151,6 @@ export const RangePopover = ({
<EuiText size="s">{ltPrependLabel}</EuiText>
</EuiToolTip>
}
fullWidth
compressed
placeholder={TO_PLACEHOLDER}
isInvalid={!isValidRange(tempRange)}
Expand Down Expand Up @@ -180,6 +179,7 @@ export const RangePopover = ({
{ defaultMessage: 'Custom label' }
)}
onSubmit={onSubmit}
compressed
dataTestSubj="indexPattern-ranges-label"
/>
</EuiFormRow>
Expand Down Expand Up @@ -284,7 +284,6 @@ export const AdvancedRangeEditor = ({
}
setLocalRanges(newRanges);
}}
formatter={formatter}
Button={({ onClick }: { onClick: MouseEventHandler }) => (
<EuiLink
color="text"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import React from 'react';
import { i18n } from '@kbn/i18n';

import { supportedFormats } from '../../../../editor_frame_service/format_column';
import { UI_SETTINGS } from '../../../../../../../../src/plugins/data/common';
import { Range } from '../../../../../../../../src/plugins/expressions/common/expression_types/index';
import { RangeEditor } from './range_editor';
Expand All @@ -32,6 +33,11 @@ export interface RangeIndexPatternColumn extends FieldBasedIndexPatternColumn {
type: MODES_TYPES;
maxBars: typeof AUTO_BARS | number;
ranges: RangeTypeLens[];
format?: { id: string; params?: { decimals: number } };
parentFormat?: {
id: string;
params?: { template?: string; id?: string; replaceInfinity?: boolean };
};
};
}

Expand Down Expand Up @@ -118,6 +124,8 @@ export const rangeOperation: OperationDefinition<RangeIndexPatternColumn, 'field
type: MODES.Histogram,
ranges: [{ from: 0, to: DEFAULT_INTERVAL, label: '' }],
maxBars: AUTO_BARS,
format: undefined,
parentFormat: undefined,
},
};
},
Expand Down Expand Up @@ -149,7 +157,25 @@ export const rangeOperation: OperationDefinition<RangeIndexPatternColumn, 'field
};
},
paramEditor: ({ state, setState, currentColumn, layerId, columnId, uiSettings, data }) => {
const rangeFormatter = data.fieldFormats.deserialize({ id: 'range' });
const numberFormat = currentColumn.params.format as
| undefined
| {
id: string;
params: { decimals: number };
};
const numberFormatterPattern =
numberFormat &&
supportedFormats[numberFormat.id] &&
supportedFormats[numberFormat.id].decimalsToPattern(numberFormat.params.decimals);

const rangeFormatter = data.fieldFormats.deserialize({
...currentColumn.params.parentFormat,
params: {
...currentColumn.params.parentFormat?.params,
...(numberFormat && { id: numberFormat.id, params: { pattern: numberFormatterPattern } }),
},
});

const MAX_HISTOGRAM_BARS = uiSettings.get(UI_SETTINGS.HISTOGRAM_MAX_BARS);
const granularityStep = (MAX_HISTOGRAM_BARS - MIN_HISTOGRAM_BARS) / SLICES;
const maxBarsDefaultValue = (MAX_HISTOGRAM_BARS - MIN_HISTOGRAM_BARS) / 2;
Expand All @@ -171,6 +197,10 @@ export const rangeOperation: OperationDefinition<RangeIndexPatternColumn, 'field
const onChangeMode = (newMode: MODES_TYPES) => {
const scale = newMode === MODES.Range ? 'ordinal' : 'interval';
const dataType = newMode === MODES.Range ? 'string' : 'number';
const parentFormat =
newMode === MODES.Range
? { id: 'range', params: { template: 'arrow_right', replaceInfinity: true } }
: undefined;
setState(
changeColumn({
state,
Expand All @@ -184,6 +214,8 @@ export const rangeOperation: OperationDefinition<RangeIndexPatternColumn, 'field
type: newMode,
ranges: [{ from: 0, to: DEFAULT_INTERVAL, label: '' }],
maxBars: maxBarsDefaultValue,
format: undefined,
parentFormat,
},
},
keepParams: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@ export const LabelInput = ({
inputRef,
onSubmit,
dataTestSubj,
compressed,
}: {
value: string;
onChange: (value: string) => void;
placeholder?: string;
inputRef?: React.MutableRefObject<HTMLInputElement | undefined>;
onSubmit?: () => void;
dataTestSubj?: string;
compressed?: boolean;
}) => {
const [inputValue, setInputValue] = useState(value);

Expand Down Expand Up @@ -57,6 +59,7 @@ export const LabelInput = ({
prepend={i18n.translate('xpack.lens.labelInput.label', {
defaultMessage: 'Label',
})}
compressed={compressed}
/>
);
};
Loading