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 35 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
100 changes: 0 additions & 100 deletions x-pack/plugins/lens/public/editor_frame_service/format_column.ts

This file was deleted.

2 changes: 0 additions & 2 deletions x-pack/plugins/lens/public/editor_frame_service/service.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import {
} from '../types';
import { Document } from '../persistence/saved_object_store';
import { mergeTables } from './merge_tables';
import { formatColumn } from './format_column';
import { EmbeddableFactory, LensEmbeddableStartServices } from './embeddable/embeddable_factory';
import { UiActionsStart } from '../../../../../src/plugins/ui_actions/public';
import { DashboardStart } from '../../../../../src/plugins/dashboard/public';
Expand Down Expand Up @@ -86,7 +85,6 @@ export class EditorFrameService {
getAttributeService: () => Promise<LensAttributeService>
): EditorFrameSetup {
plugins.expressions.registerFunction(() => mergeTables);
plugins.expressions.registerFunction(() => formatColumn);

const getStartServices = async (): Promise<LensEmbeddableStartServices> => {
const [coreStart, deps] = await core.getStartServices();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,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
137 changes: 137 additions & 0 deletions x-pack/plugins/lens/public/indexpattern_datasource/format_column.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import {
ExpressionFunctionDefinition,
Datatable,
DatatableColumn,
} from 'src/plugins/expressions/public';

interface FormatColumn {
format: string;
columnId: string;
decimals?: number;
parentFormat?: string;
}

export const supportedFormats: Record<
string,
{ decimalsToPattern: (decimals?: number) => string }
> = {
number: {
decimalsToPattern: (decimals = 2) => {
if (decimals === 0) {
return `0,0`;
}
return `0,0.${'0'.repeat(decimals)}`;
},
},
percent: {
decimalsToPattern: (decimals = 2) => {
if (decimals === 0) {
return `0,0%`;
}
return `0,0.${'0'.repeat(decimals)}%`;
},
},
bytes: {
decimalsToPattern: (decimals = 2) => {
if (decimals === 0) {
return `0,0b`;
}
return `0,0.${'0'.repeat(decimals)}b`;
},
},
};

export const formatColumn: ExpressionFunctionDefinition<
'lens_format_column',
Datatable,
FormatColumn,
Datatable
> = {
name: 'lens_format_column',
type: 'datatable',
help: '',
args: {
format: {
types: ['string'],
help: '',
required: true,
},
columnId: {
types: ['string'],
help: '',
required: true,
},
decimals: {
types: ['number'],
help: '',
},
parentFormat: {
types: ['string'],
help: '',
},
},
inputTypes: ['datatable'],
fn(input, { format, columnId, decimals, parentFormat }: FormatColumn) {
return {
...input,
columns: input.columns.map((col) => {
if (col.id === columnId) {
if (!parentFormat) {
if (supportedFormats[format]) {
return withParams(col, {
id: format,
params: { pattern: supportedFormats[format].decimalsToPattern(decimals) },
});
} else if (format) {
return withParams(col, { id: format });
} else {
return col;
}
}

const parsedParentFormat = JSON.parse(parentFormat);
const parentFormatId = parsedParentFormat.id;
const parentFormatParams = parsedParentFormat.params ?? {};

if (!parentFormatId) {
return col;
}

if (format && supportedFormats[format]) {
return withParams(col, {
id: parentFormatId,
params: {
id: format,
params: {
pattern: supportedFormats[format].decimalsToPattern(decimals),
},
...parentFormatParams,
},
});
}
if (parentFormatParams) {
const innerParams = (col.meta.params?.params as Record<string, unknown>) ?? {};
return withParams(col, {
...col.meta.params,
params: {
...innerParams,
...parentFormatParams,
},
});
}
}
return col;
}),
};
},
};

function withParams(col: DatatableColumn, params: Record<string, unknown>) {
return { ...col, meta: { ...col.meta, params } };
}
2 changes: 2 additions & 0 deletions x-pack/plugins/lens/public/indexpattern_datasource/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
DataPublicPluginStart,
} from '../../../../../src/plugins/data/public';
import { Datasource, EditorFrameSetup } from '../types';
import { formatColumn } from './format_column';

export interface IndexPatternDatasourceSetupPlugins {
expressions: ExpressionsSetup;
Expand All @@ -35,6 +36,7 @@ export class IndexPatternDatasource {
editorFrame.registerDatasource(async () => {
const { getIndexPatternDatasource, renameColumns } = await import('../async_services');
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the new setup of making this an implementation detail of the datasource - can we move the formatColumn import into the async_services so it's loaded async?

expressions.registerFunction(renameColumns);
expressions.registerFunction(formatColumn);
return core.getStartServices().then(([coreStart, { data }]) =>
getIndexPatternDatasource({
core: coreStart,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ const indexPattern2 = ({
title: 'my-fake-restricted-pattern',
timeFieldName: 'timestamp',
hasRestrictions: true,
fieldFormatMap: { bytes: { id: 'bytes', params: { pattern: '0.0' } } },
fields: [
{
name: 'timestamp',
Expand Down
9 changes: 8 additions & 1 deletion x-pack/plugins/lens/public/indexpattern_datasource/loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,14 @@ export async function loadIndexPatterns({
id: indexPattern.id!, // id exists for sure because we got index patterns by id
title,
timeFieldName,
fieldFormatMap,
fieldFormatMap:
fieldFormatMap &&
Object.fromEntries(
Object.entries(fieldFormatMap).map(([id, format]) => [
id,
'toJSON' in format ? format.toJSON() : format,
])
),
fields: newFields,
hasRestrictions: !!typeMeta?.aggs,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export const createMockedRestrictedIndexPattern = () => ({
title: 'my-fake-restricted-pattern',
timeFieldName: 'timestamp',
hasRestrictions: true,
fieldFormatMap: { bytes: { id: 'bytes', params: { pattern: '0.0' } } },
fields: [
{
name: 'timestamp',
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: 14ch; // Roughly 10 characters plus extra for the padding
}
Loading