Skip to content

Commit

Permalink
[Lens][Datatable] Fix non-numeric default cell text alignment (elasti…
Browse files Browse the repository at this point in the history
…c#193886)

Fixes elastic#191258 where the default alignment of the cell was different between the dimension editor and the table vis.
- Assigns default alignment of `'right'` for all number values excluding `ranges`, `multi_terms`, `filters` and `filtered_metric`. Otherwise assigns `'left'`.
- The default alignment is never save until the user changes it themselves.
  • Loading branch information
nickofthyme authored Oct 10, 2024
1 parent a397bb7 commit db54cb1
Show file tree
Hide file tree
Showing 17 changed files with 196 additions and 109 deletions.
31 changes: 27 additions & 4 deletions x-pack/plugins/lens/common/expressions/datatable/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,37 @@
* 2.0.
*/

import type { Datatable } from '@kbn/expressions-plugin/common';
import { type Datatable, type DatatableColumnMeta } from '@kbn/expressions-plugin/common';
import { getOriginalId } from './transpose_helpers';

/**
* Returns true for numerical fields
*
* Excludes the following types:
* - `range` - Stringified range
* - `multi_terms` - Multiple values
* - `filters` - Arbitrary label
* - `filtered_metric` - Array of values
*/
export function isNumericField(meta?: DatatableColumnMeta): boolean {
return (
meta?.type === 'number' &&
meta.params?.id !== 'range' &&
meta.params?.id !== 'multi_terms' &&
meta.sourceParams?.type !== 'filters' &&
meta.sourceParams?.type !== 'filtered_metric'
);
}

/**
* Returns true for numerical fields, excluding ranges
*/
export function isNumericFieldForDatatable(table: Datatable | undefined, accessor: string) {
return getFieldTypeFromDatatable(table, accessor) === 'number';
const meta = getFieldMetaFromDatatable(table, accessor);
return isNumericField(meta);
}

export function getFieldTypeFromDatatable(table: Datatable | undefined, accessor: string) {
export function getFieldMetaFromDatatable(table: Datatable | undefined, accessor: string) {
return table?.columns.find((col) => col.id === accessor || getOriginalId(col.id) === accessor)
?.meta.type;
?.meta;
}
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ describe('findMinMaxByColumnId', () => {
{ a: 'shoes', b: 53 },
],
})
).toEqual({ b: { min: 2, max: 53 } });
).toEqual(new Map([['b', { min: 2, max: 53 }]]));
});
});

Expand Down
21 changes: 12 additions & 9 deletions x-pack/plugins/lens/public/shared_components/coloring/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,32 +95,35 @@ export const findMinMaxByColumnId = (
table: Datatable | undefined,
getOriginalId: (id: string) => string = (id: string) => id
) => {
const minMax: Record<string, DataBounds> = {};
const minMaxMap = new Map<string, DataBounds>();

if (table != null) {
for (const columnId of columnIds) {
const originalId = getOriginalId(columnId);
minMax[originalId] = minMax[originalId] || {
const minMax = minMaxMap.get(originalId) ?? {
max: Number.NEGATIVE_INFINITY,
min: Number.POSITIVE_INFINITY,
};
table.rows.forEach((row) => {
const rowValue = row[columnId];
const numericValue = getNumericValue(rowValue);
if (numericValue != null) {
if (minMax[originalId].min > numericValue) {
minMax[originalId].min = numericValue;
if (minMax.min > numericValue) {
minMax.min = numericValue;
}
if (minMax[originalId].max < numericValue) {
minMax[originalId].max = numericValue;
if (minMax.max < numericValue) {
minMax.max = numericValue;
}
}
});

// what happens when there's no data in the table? Fallback to a percent range
if (minMax[originalId].max === Number.NEGATIVE_INFINITY) {
minMax[originalId] = getFallbackDataBounds();
if (minMax.max === Number.NEGATIVE_INFINITY) {
minMaxMap.set(originalId, getFallbackDataBounds());
} else {
minMaxMap.set(originalId, minMax);
}
}
}
return minMax;
return minMaxMap;
};
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,7 @@ describe('datatable cell renderer', () => {
<DataContext.Provider
value={{
table,
alignments: {
a: 'right',
},
alignments: new Map([['a', 'right']]),
...wrapperProps,
}}
>
Expand Down Expand Up @@ -217,7 +215,7 @@ describe('datatable cell renderer', () => {
{
wrapper: DataContextProviderWrapper({
table,
minMaxByColumnId: { a: { min: 12, max: 155 } },
minMaxByColumnId: new Map([['a', { min: 12, max: 155 }]]),
...context,
}),
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export const createGridCell = (
} = columnConfig.columns[colIndex] ?? {};
const filterOnClick = oneClickFilter && handleFilterClick;
const content = formatters[columnId]?.convert(rawRowValue, filterOnClick ? 'text' : 'html');
const currentAlignment = alignments && alignments[columnId];
const currentAlignment = alignments?.get(columnId);

useEffect(() => {
let colorSet = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ const callCreateGridColumns = (
params.formatFactory ?? (((x: unknown) => ({ convert: () => x })) as unknown as FormatFactory),
params.onColumnResize ?? jest.fn(),
params.onColumnHide ?? jest.fn(),
params.alignments ?? {},
params.alignments ?? new Map(),
params.headerRowHeight ?? RowHeightMode.auto,
params.headerRowLines ?? 1,
params.columnCellValueActions ?? [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export const createGridColumns = (
formatFactory: FormatFactory,
onColumnResize: (eventData: { columnId: string; width: number | undefined }) => void,
onColumnHide: ((eventData: { columnId: string }) => void) | undefined,
alignments: Record<string, 'left' | 'right' | 'center'>,
alignments: Map<string, 'left' | 'right' | 'center'>,
headerRowHeight: RowHeightMode,
headerRowLines: number,
columnCellValueActions: LensCellValueAction[][] | undefined,
Expand Down Expand Up @@ -261,7 +261,7 @@ export const createGridColumns = (
});
}
}
const currentAlignment = alignments && alignments[field];
const currentAlignment = alignments && alignments.get(field);
const hasMultipleRows = [RowHeightMode.auto, RowHeightMode.custom, undefined].includes(
headerRowHeight
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,20 @@
*/

import React from 'react';
import { DEFAULT_COLOR_MAPPING_CONFIG, type PaletteRegistry } from '@kbn/coloring';
import { DEFAULT_COLOR_MAPPING_CONFIG } from '@kbn/coloring';
import { act, render, screen } from '@testing-library/react';
import userEvent, { type UserEvent } from '@testing-library/user-event';
import { chartPluginMock } from '@kbn/charts-plugin/public/mocks';
import { LayerTypes } from '@kbn/expression-xy-plugin/public';
import { EuiButtonGroupTestHarness } from '@kbn/test-eui-helpers';
import {
FramePublicAPI,
OperationDescriptor,
VisualizationDimensionEditorProps,
DatasourcePublicAPI,
DataType,
} from '../../../types';
import { FramePublicAPI, DatasourcePublicAPI, OperationDescriptor } from '../../../types';
import { DatatableVisualizationState } from '../visualization';
import { createMockDatasource, createMockFramePublicAPI } from '../../../mocks';
import { TableDimensionEditor } from './dimension_editor';
import { TableDimensionEditor, TableDimensionEditorProps } from './dimension_editor';
import { ColumnState } from '../../../../common/expressions';
import { capitalize } from 'lodash';
import { I18nProvider } from '@kbn/i18n-react';
import { DatatableColumnType } from '@kbn/expressions-plugin/common';

describe('data table dimension editor', () => {
let user: UserEvent;
Expand All @@ -35,10 +30,8 @@ describe('data table dimension editor', () => {
alignment: EuiButtonGroupTestHarness;
};
let mockOperationForFirstColumn: (overrides?: Partial<OperationDescriptor>) => void;
let props: VisualizationDimensionEditorProps<DatatableVisualizationState> & {
paletteService: PaletteRegistry;
isDarkMode: boolean;
};

let props: TableDimensionEditorProps;

function testState(): DatatableVisualizationState {
return {
Expand Down Expand Up @@ -80,6 +73,7 @@ describe('data table dimension editor', () => {
name: 'foo',
meta: {
type: 'string',
params: {},
},
},
],
Expand Down Expand Up @@ -114,13 +108,7 @@ describe('data table dimension editor', () => {
mockOperationForFirstColumn();
});

const renderTableDimensionEditor = (
overrideProps?: Partial<
VisualizationDimensionEditorProps<DatatableVisualizationState> & {
paletteService: PaletteRegistry;
}
>
) => {
const renderTableDimensionEditor = (overrideProps?: Partial<TableDimensionEditorProps>) => {
return render(<TableDimensionEditor {...props} {...overrideProps} />, {
wrapper: ({ children }) => (
<I18nProvider>
Expand All @@ -137,11 +125,18 @@ describe('data table dimension editor', () => {
});

it('should render default alignment for number', () => {
mockOperationForFirstColumn({ dataType: 'number' });
frame.activeData!.first.columns[0].meta.type = 'number';
renderTableDimensionEditor();
expect(btnGroups.alignment.selected).toHaveTextContent('Right');
});

it('should render default alignment for ranges', () => {
frame.activeData!.first.columns[0].meta.type = 'number';
frame.activeData!.first.columns[0].meta.params = { id: 'range' };
renderTableDimensionEditor();
expect(btnGroups.alignment.selected).toHaveTextContent('Left');
});

it('should render specific alignment', () => {
state.columns[0].alignment = 'center';
renderTableDimensionEditor();
Expand Down Expand Up @@ -181,10 +176,11 @@ describe('data table dimension editor', () => {
expect(screen.queryByTestId('lns_dynamicColoring_edit')).not.toBeInTheDocument();
});

it.each<DataType>(['date'])(
it.each<DatatableColumnType>(['date'])(
'should not show the dynamic coloring option for "%s" columns',
(dataType) => {
mockOperationForFirstColumn({ dataType });
(type) => {
frame.activeData!.first.columns[0].meta.type = type;

renderTableDimensionEditor();
expect(screen.queryByTestId('lnsDatatable_dynamicColoring_groups')).not.toBeInTheDocument();
expect(screen.queryByTestId('lns_dynamicColoring_edit')).not.toBeInTheDocument();
Expand Down Expand Up @@ -231,15 +227,16 @@ describe('data table dimension editor', () => {
});
});

it.each<{ flyout: 'terms' | 'values'; isBucketed: boolean; dataType: DataType }>([
{ flyout: 'terms', isBucketed: true, dataType: 'number' },
{ flyout: 'terms', isBucketed: false, dataType: 'string' },
{ flyout: 'values', isBucketed: false, dataType: 'number' },
it.each<{ flyout: 'terms' | 'values'; isBucketed: boolean; type: DatatableColumnType }>([
{ flyout: 'terms', isBucketed: true, type: 'number' },
{ flyout: 'terms', isBucketed: false, type: 'string' },
{ flyout: 'values', isBucketed: false, type: 'number' },
])(
'should show color by $flyout flyout when bucketing is $isBucketed with $dataType column',
async ({ flyout, isBucketed, dataType }) => {
'should show color by $flyout flyout when bucketing is $isBucketed with $type column',
async ({ flyout, isBucketed, type }) => {
state.columns[0].colorMode = 'cell';
mockOperationForFirstColumn({ isBucketed, dataType });
frame.activeData!.first.columns[0].meta.type = type;
mockOperationForFirstColumn({ isBucketed });
renderTableDimensionEditor();

await user.click(screen.getByLabelText('Edit colors'));
Expand All @@ -251,6 +248,7 @@ describe('data table dimension editor', () => {

it('should show the dynamic coloring option for a bucketed operation', () => {
state.columns[0].colorMode = 'cell';
frame.activeData!.first.columns[0].meta.type = 'string';
mockOperationForFirstColumn({ isBucketed: true });

renderTableDimensionEditor();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import React, { useCallback } from 'react';
import { i18n } from '@kbn/i18n';
import { EuiFormRow, EuiSwitch, EuiButtonGroup, htmlIdGenerator } from '@elastic/eui';
import { PaletteRegistry } from '@kbn/coloring';
import { PaletteRegistry, getFallbackDataBounds } from '@kbn/coloring';
import { getColorCategories } from '@kbn/chart-expressions-common';
import { useDebouncedValue } from '@kbn/visualization-utils';
import type { VisualizationDimensionEditorProps } from '../../../types';
Expand All @@ -26,6 +26,11 @@ import './dimension_editor.scss';
import { CollapseSetting } from '../../../shared_components/collapse_setting';
import { ColorMappingByValues } from '../../../shared_components/coloring/color_mapping_by_values';
import { ColorMappingByTerms } from '../../../shared_components/coloring/color_mapping_by_terms';
import { getColumnAlignment } from '../utils';
import {
getFieldMetaFromDatatable,
isNumericField,
} from '../../../../common/expressions/datatable/utils';

const idPrefix = htmlIdGenerator()();

Expand All @@ -45,12 +50,13 @@ function updateColumn(
});
}

export function TableDimensionEditor(
props: VisualizationDimensionEditorProps<DatatableVisualizationState> & {
export type TableDimensionEditorProps =
VisualizationDimensionEditorProps<DatatableVisualizationState> & {
paletteService: PaletteRegistry;
isDarkMode: boolean;
}
) {
};

export function TableDimensionEditor(props: TableDimensionEditorProps) {
const { frame, accessor, isInlineEditing, isDarkMode } = props;
const column = props.state.columns.find(({ columnId }) => accessor === columnId);
const { inputValue: localState, handleInputChange: setLocalState } =
Expand All @@ -74,12 +80,13 @@ export function TableDimensionEditor(

const currentData = frame.activeData?.[localState.layerId];
const datasource = frame.datasourceLayers?.[localState.layerId];
const { dataType, isBucketed } = datasource?.getOperationForColumnId(accessor) ?? {};
const showColorByTerms = shouldColorByTerms(dataType, isBucketed);
const currentAlignment = column?.alignment || (dataType === 'number' ? 'right' : 'left');
const { isBucketed } = datasource?.getOperationForColumnId(accessor) ?? {};
const meta = getFieldMetaFromDatatable(currentData, accessor);
const showColorByTerms = shouldColorByTerms(meta?.type, isBucketed);
const currentAlignment = getColumnAlignment(column, isNumericField(meta));
const currentColorMode = column?.colorMode || 'none';
const hasDynamicColoring = currentColorMode !== 'none';
const showDynamicColoringFeature = dataType !== 'date';
const showDynamicColoringFeature = meta?.type !== 'date';
const visibleColumnsCount = localState.columns.filter((c) => !c.hidden).length;

const hasTransposedColumn = localState.columns.some(({ isTransposed }) => isTransposed);
Expand All @@ -88,7 +95,7 @@ export function TableDimensionEditor(
[]
: [accessor];
const minMaxByColumnId = findMinMaxByColumnId(columnsToCheck, currentData, getOriginalId);
const currentMinMax = minMaxByColumnId[accessor];
const currentMinMax = minMaxByColumnId.get(accessor) ?? getFallbackDataBounds();

const activePalette = column?.palette ?? {
type: 'palette',
Expand Down
Loading

0 comments on commit db54cb1

Please sign in to comment.