chore: address simple pr comments

elastic · Dec 10, 2024 · 016f161 · 016f161
1 parent 6afbfe1
commit 016f161
Show file tree

Hide file tree

Showing 14 changed files with 66 additions and 91 deletions.
diff --git a/packages/kbn-coloring/src/shared_components/color_mapping/color/color_handling.test.ts b/packages/kbn-coloring/src/shared_components/color_mapping/color/color_handling.test.ts
@@ -340,9 +340,9 @@ describe('Color mapping - color generation', () => {
         categories: ['cat1', 'cat2', 'cat3'],
       }
     );
-    expect(toHex(colorFactory('cat1'))).toBe(toHex(elasticPaletteColors[0])); // EUI green
-    expect(toHex(colorFactory('cat2'))).toBe('#a4908f'); // red gray green
-    expect(toHex(colorFactory('cat3'))).toBe(toHex(elasticPaletteColors[2])); // EUI pink
+    expect(toHex(colorFactory('cat1'))).toBe(toHex(elasticPaletteColors[0]));
+    expect(toHex(colorFactory('cat2'))).toBe('#a4908f');
+    expect(toHex(colorFactory('cat3'))).toBe(toHex(elasticPaletteColors[2]));
   });
 
   it('returns divergent gradient [asc, darkMode]', () => {
@@ -376,10 +376,10 @@ describe('Color mapping - color generation', () => {
         categories: ['cat1', 'cat2', 'cat3'],
       }
     );
-    expect(toHex(colorFactory('cat1'))).toBe(toHex(elasticPaletteColors[2])); // EUI pink
-    expect(toHex(colorFactory('cat2'))).toBe(neutralPaletteColors[0]); // NEUTRAL LIGHT GRAY
-    expect(toHex(colorFactory('cat3'))).toBe(toHex(elasticPaletteColors[0])); // EUI green
-    // if the category is not available in the `categories` list then a default netural is used
+    expect(toHex(colorFactory('cat1'))).toBe(toHex(elasticPaletteColors[2]));
+    expect(toHex(colorFactory('cat2'))).toBe(neutralPaletteColors[0]);
+    expect(toHex(colorFactory('cat3'))).toBe(toHex(elasticPaletteColors[0]));
+    // if the category is not available in the `categories` list then a default neutral is used
     // this is an edge case and ideally never happen
     expect(colorFactory('not_available')).toBe(neutralPaletteColors[DEFAULT_NEUTRAL_PALETTE_INDEX]);
   });

diff --git a/packages/kbn-palettes/classes/categorical_palette.ts b/packages/kbn-palettes/classes/categorical_palette.ts
@@ -26,7 +26,7 @@ export class KbnCategoricalPalette extends KbnBasePalette implements IKbnPalette
     this.#colors = colors;
   }
 
-  public colors = (n?: number) => {
-    return this.#colors.slice(0, n);
+  public colors = (n: number = 1) => {
+    return this.#colors.slice(0, Math.max(1, n));
   };
 }
diff --git a/packages/kbn-palettes/classes/color_fn_palette.ts b/packages/kbn-palettes/classes/color_fn_palette.ts
@@ -41,6 +41,6 @@ export class KbnColorFnPalette extends KbnBasePalette implements IKbnPalette {
   }
 
   public colors = (n: number = this.#defaultNumberOfColors) => {
-    return this.#colorFn(n);
+    return this.#colorFn(Math.max(1, n));
   };
 }
diff --git a/packages/kbn-palettes/classes/gradient_palette.ts b/packages/kbn-palettes/classes/gradient_palette.ts
diff --git a/packages/kbn-palettes/classes/types.ts b/packages/kbn-palettes/classes/types.ts
@@ -39,6 +39,8 @@ export interface IKbnPalette {
   aliases: string[];
   /**
    * Excluded from `getAll` but can still query for palette with `get`/`query`
+   *
+   * An example would be `KbnPalette.Neutral` palette. I want to exclude it from the list of all available palettes, but I still want to `get`/`query` the palette.
    */
   standalone?: boolean;
   /**

diff --git a/packages/kbn-palettes/constants.ts b/packages/kbn-palettes/constants.ts
@@ -63,22 +63,13 @@ const gradient = {
  * Enum of all kbn palette ids, including by type
  */
 export const KbnPalette = {
-  /**
-   * Categorical palettes
-   */
-  _categorical: categorical,
+  // Categorical palettes
   ...categorical,
 
-  /**
-   * Gradient palettes
-   */
-  _gradient: gradient,
+  // Gradient palettes
   ...gradient,
 
-  /**
-   * Semantic palettes
-   */
-  _semantic: semantic,
+  // Semantic palettes
   ...semantic,
 
   // ---- Deprecated palettes ----

diff --git a/packages/kbn-palettes/palettes/categorical/neutral.ts b/packages/kbn-palettes/palettes/categorical/neutral.ts
@@ -8,18 +8,29 @@
  */
 
 import { i18n } from '@kbn/i18n';
-import { KbnCategoricalPalette } from '../../classes/categorical_palette';
+import {
+  KbnCategoricalPalette,
+  KbnCategoricalPaletteConfig,
+} from '../../classes/categorical_palette';
 import { KbnPalette } from '../../constants';
 
-const lightColors = ['#F6F9FC', '#D0D4DA', '#989FAA', '#666D78', '#373D45'];
-const darkColors = ['#F6F9FC', '#C9D4E6', '#89A0C4', '#546D95', '#283C5C'];
+const commonProps = {
+  id: KbnPalette.Neutral,
+  standalone: true,
+  name: i18n.translate('palettes.elastic.name', {
+    defaultMessage: 'Neutral',
+  }),
+} satisfies Omit<KbnCategoricalPaletteConfig, 'colors'>;
+
+const lightNeutralPalette = new KbnCategoricalPalette({
+  ...commonProps,
+  colors: ['#F6F9FC', '#D0D4DA', '#989FAA', '#666D78', '#373D45'],
+});
+
+const darkNeutralPalette = new KbnCategoricalPalette({
+  ...commonProps,
+  colors: ['#F6F9FC', '#C9D4E6', '#89A0C4', '#546D95', '#283C5C'],
+});
 
 export const getNeutralPalette = (darkMode: boolean) =>
-  new KbnCategoricalPalette({
-    id: KbnPalette.Neutral,
-    standalone: true,
-    name: i18n.translate('palettes.elastic.name', {
-      defaultMessage: 'Neutral',
-    }),
-    colors: darkMode ? darkColors : lightColors,
-  });
+  darkMode ? darkNeutralPalette : lightNeutralPalette;
diff --git a/packages/kbn-palettes/types.ts b/packages/kbn-palettes/types.ts
diff --git a/x-pack/plugins/lens/public/shared_components/coloring/color_mapping_by_terms.tsx b/x-pack/plugins/lens/public/shared_components/coloring/color_mapping_by_terms.tsx
@@ -30,7 +30,7 @@ import { KbnPalettes } from '@kbn/palettes';
 import { trackUiCounterEvents } from '../../lens_ui_telemetry';
 import { PalettePicker } from '../palette_picker';
 import { PalettePanelContainer } from './palette_panel_container';
-import { getColorStops } from './utils';
+import { getPaletteDisplayColors } from './utils';
 
 interface ColorMappingByTermsProps {
   isDarkMode: boolean;
@@ -69,7 +69,13 @@ export function ColorMappingByTerms({
       fullWidth
     >
       <PalettePanelContainer
-        palette={getColorStops(paletteService, palettes, isDarkMode, palette, colorMapping)}
+        palette={getPaletteDisplayColors(
+          paletteService,
+          palettes,
+          isDarkMode,
+          palette,
+          colorMapping
+        )}
         siblingRef={panelRef}
         title={
           useNewColorMapping

diff --git a/x-pack/plugins/lens/public/shared_components/coloring/utils.ts b/x-pack/plugins/lens/public/shared_components/coloring/utils.ts
@@ -29,7 +29,7 @@ import { DataType } from '../../types';
 /**
  * Returns array of colors for provided palette or colorMapping
  */
-export function getColorStops(
+export function getPaletteDisplayColors(
   paletteService: PaletteRegistry,
   palettes: KbnPalettes,
   isDarkMode: boolean,

diff --git a/x-pack/plugins/lens/public/visualizations/datatable/visualization.test.tsx b/x-pack/plugins/lens/public/visualizations/datatable/visualization.test.tsx
@@ -27,13 +27,13 @@ import {
   DatatableColumnFn,
   DatatableExpressionFunction,
 } from '../../../common/expressions';
-import { getColorStops } from '../../shared_components/coloring';
+import { getPaletteDisplayColors } from '../../shared_components/coloring';
 import { fieldFormatsServiceMock } from '@kbn/field-formats-plugin/public/mocks';
 
 jest.mock('../../shared_components/coloring', () => {
   return {
     ...jest.requireActual('../../shared_components/coloring'),
-    getColorStops: jest.fn().mockReturnValue([]),
+    getPaletteDisplayColors: jest.fn().mockReturnValue([]),
   };
 });
 
@@ -438,7 +438,7 @@ describe('Datatable Visualization', () => {
       let params: VisualizationConfigProps<DatatableVisualizationState>;
 
       beforeEach(() => {
-        (getColorStops as jest.Mock).mockReturnValue(mockStops);
+        (getPaletteDisplayColors as jest.Mock).mockReturnValue(mockStops);
       });
 
       describe('rows', () => {
@@ -483,7 +483,7 @@ describe('Datatable Visualization', () => {
         it.each<ColumnState['colorMode']>(['cell', 'text'])(
           'should not include palette if colorMode is %s but stops is empty',
           (colorMode) => {
-            (getColorStops as jest.Mock).mockReturnValue([]);
+            (getPaletteDisplayColors as jest.Mock).mockReturnValue([]);
             params.state.columns[0].colorMode = colorMode;
             expect(datatableVisualization.getConfiguration(params).groups[0].accessors).toEqual([
               { columnId: 'b' },
@@ -532,7 +532,7 @@ describe('Datatable Visualization', () => {
         it.each<ColumnState['colorMode']>(['cell', 'text'])(
           'should not include palette if colorMode is %s but stops is empty',
           (colorMode) => {
-            (getColorStops as jest.Mock).mockReturnValue([]);
+            (getPaletteDisplayColors as jest.Mock).mockReturnValue([]);
             params.state.columns[0].colorMode = colorMode;
             expect(datatableVisualization.getConfiguration(params).groups[2].accessors).toEqual([
               { columnId: 'b' },

diff --git a/x-pack/plugins/lens/public/visualizations/datatable/visualization.tsx b/x-pack/plugins/lens/public/visualizations/datatable/visualization.tsx
@@ -55,7 +55,7 @@ import {
 import {
   defaultPaletteParams,
   findMinMaxByColumnId,
-  getColorStops,
+  getPaletteDisplayColors,
   shouldColorByTerms,
 } from '../../shared_components';
 import { getColorMappingTelemetryEvents } from '../../lens_ui_telemetry/color_telemetry_helpers';
@@ -329,7 +329,7 @@ export const getDatatableVisualization = ({
                 hidden,
                 collapseFn,
               } = columnMap[accessor] ?? {};
-              const stops = getColorStops(
+              const stops = getPaletteDisplayColors(
                 paletteService,
                 palettes,
                 theme.darkMode,
@@ -422,7 +422,7 @@ export const getDatatableVisualization = ({
                 colorMapping,
                 hidden,
               } = columnMap[accessor] ?? {};
-              const stops = getColorStops(
+              const stops = getPaletteDisplayColors(
                 paletteService,
                 palettes,
                 theme.darkMode,

diff --git a/x-pack/plugins/lens/public/visualizations/partition/dimension_editor.tsx b/x-pack/plugins/lens/public/visualizations/partition/dimension_editor.tsx
@@ -23,7 +23,11 @@ import { getColorCategories } from '@kbn/chart-expressions-common';
 import { KbnPalette, KbnPalettes } from '@kbn/palettes';
 import { PieVisualizationState } from '../../../common/types';
 import { VisualizationDimensionEditorProps } from '../../types';
-import { PalettePanelContainer, PalettePicker, getColorStops } from '../../shared_components';
+import {
+  PalettePanelContainer,
+  PalettePicker,
+  getPaletteDisplayColors,
+} from '../../shared_components';
 import { CollapseSetting } from '../../shared_components/collapse_setting';
 import {
   getDefaultColorForMultiMetricDimension,
@@ -118,7 +122,7 @@ export function DimensionEditor(props: DimensionEditorProps) {
         })
     : undefined;
 
-  const colors = getColorStops(
+  const colors = getPaletteDisplayColors(
     props.paletteService,
     props.palettes,
     props.isDarkMode,

diff --git a/x-pack/plugins/lens/public/visualizations/tagcloud/tags_dimension_editor.tsx b/x-pack/plugins/lens/public/visualizations/tagcloud/tags_dimension_editor.tsx
@@ -21,7 +21,11 @@ import { useDebouncedValue } from '@kbn/visualization-utils';
 import { getColorCategories } from '@kbn/chart-expressions-common';
 import { KbnPalettes } from '@kbn/palettes';
 import type { TagcloudState } from './types';
-import { PalettePanelContainer, PalettePicker, getColorStops } from '../../shared_components';
+import {
+  PalettePanelContainer,
+  PalettePicker,
+  getPaletteDisplayColors,
+} from '../../shared_components';
 import { FramePublicAPI } from '../../types';
 import { trackUiCounterEvents } from '../../lens_ui_telemetry';
 
@@ -53,7 +57,7 @@ export function TagsDimensionEditor({
     });
   const [useNewColorMapping, setUseNewColorMapping] = useState(state.colorMapping ? true : false);
 
-  const colors = getColorStops(
+  const colors = getPaletteDisplayColors(
     paletteService,
     palettes,
     isDarkMode,