[Lens][Datatable] Fix non-numeric default cell text alignment (#193886)

Fixes #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.
elastic · Oct 10, 2024 · db54cb1 · db54cb1
1 parent a397bb7
commit db54cb1
Show file tree

Hide file tree

Showing 17 changed files with 196 additions and 109 deletions.
diff --git a/x-pack/plugins/lens/common/expressions/datatable/utils.ts b/x-pack/plugins/lens/common/expressions/datatable/utils.ts
@@ -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;
 }
diff --git a/x-pack/plugins/lens/public/shared_components/coloring/utils.test.ts b/x-pack/plugins/lens/public/shared_components/coloring/utils.test.ts
@@ -110,7 +110,7 @@ describe('findMinMaxByColumnId', () => {
           { a: 'shoes', b: 53 },
         ],
       })
-    ).toEqual({ b: { min: 2, max: 53 } });
+    ).toEqual(new Map([['b', { min: 2, max: 53 }]]));
   });
 });
 

diff --git a/x-pack/plugins/lens/public/shared_components/coloring/utils.ts b/x-pack/plugins/lens/public/shared_components/coloring/utils.ts
@@ -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;
 };
diff --git a/x-pack/plugins/lens/public/visualizations/datatable/components/cell_value.test.tsx b/x-pack/plugins/lens/public/visualizations/datatable/components/cell_value.test.tsx
@@ -54,9 +54,7 @@ describe('datatable cell renderer', () => {
         <DataContext.Provider
           value={{
             table,
-            alignments: {
-              a: 'right',
-            },
+            alignments: new Map([['a', 'right']]),
             ...wrapperProps,
           }}
         >
@@ -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,
           }),
         }

diff --git a/x-pack/plugins/lens/public/visualizations/datatable/components/cell_value.tsx b/x-pack/plugins/lens/public/visualizations/datatable/components/cell_value.tsx
@@ -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;

diff --git a/x-pack/plugins/lens/public/visualizations/datatable/components/columns.test.tsx b/x-pack/plugins/lens/public/visualizations/datatable/components/columns.test.tsx
@@ -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 ?? [],

diff --git a/x-pack/plugins/lens/public/visualizations/datatable/components/columns.tsx b/x-pack/plugins/lens/public/visualizations/datatable/components/columns.tsx
@@ -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,
@@ -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
     );

diff --git a/x-pack/plugins/lens/public/visualizations/datatable/components/dimension_editor.test.tsx b/x-pack/plugins/lens/public/visualizations/datatable/components/dimension_editor.test.tsx
@@ -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;
@@ -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 {
@@ -80,6 +73,7 @@ describe('data table dimension editor', () => {
             name: 'foo',
             meta: {
               type: 'string',
+              params: {},
             },
           },
         ],
@@ -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>
@@ -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();
@@ -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();
@@ -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'));
@@ -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();

diff --git a/x-pack/plugins/lens/public/visualizations/datatable/components/dimension_editor.tsx b/x-pack/plugins/lens/public/visualizations/datatable/components/dimension_editor.tsx
@@ -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';
@@ -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()();
 
@@ -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 } =
@@ -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);
@@ -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',
-Original file 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 @@