revert(legend): change click on item behaviour (#2429)

Revert "feat(Legend): change click on item behaviour (#2427)" This reverts commit b1c72df.
elastic · May 16, 2024 · cc438a1 · cc438a1
1 parent b1c72df
commit cc438a1
Show file tree

Hide file tree

Showing 9 changed files with 90 additions and 135 deletions.
diff --git a/e2e/page_objects/common.ts b/e2e/page_objects/common.ts
@@ -44,10 +44,10 @@ interface ElementBBox {
   height: number;
 }
 
-type KeyboardKey = {
+interface KeyboardKey {
   key: string;
   count: number;
-};
+}
 
 interface ClickOptions {
   /**
@@ -217,13 +217,6 @@ export class CommonPage {
     ];
   }
 
-  getModifierKey = (page: Page) => async () => {
-    const isMac = await page.evaluate(() => {
-      return navigator.userAgent.includes('Mac');
-    });
-    return isMac ? 'Meta' : 'Control';
-  };
-
   /**
    * Toggle element visibility
    * @param selector
@@ -358,12 +351,18 @@ export class CommonPage {
    */
   // eslint-disable-next-line class-methods-use-this
   pressKey = (page: Page) => async (key: string, count: number) => {
-    // capitalize some keys
-    const keyName = ['tab', 'enter'].includes(key) ? `${key.charAt(0).toUpperCase()}${key.slice(1)}` : key;
-    let i = 0;
-    while (i < count) {
-      await page.keyboard.press(keyName);
-      i++;
+    if (key === 'tab') {
+      let i = 0;
+      while (i < count) {
+        await page.keyboard.press('Tab');
+        i++;
+      }
+    } else if (key === 'enter') {
+      let i = 0;
+      while (i < count) {
+        await page.keyboard.press('Enter');
+        i++;
+      }
     }
   };
 

diff --git a/e2e/tests/area_stories.test.ts b/e2e/tests/area_stories.test.ts
@@ -64,9 +64,7 @@ test.describe('Area series stories', () => {
 
     test('shows only positive values when hiding negative one', async ({ page }) => {
       const action = async () => {
-        await page.keyboard.down(await common.getModifierKey(page)());
         await page.click('.echLegendItem:nth-child(2) .echLegendItem__label');
-        await page.keyboard.up(await common.getModifierKey(page)());
       };
       await common.expectChartAtUrlToMatchScreenshot(page)(
         'http://localhost:9001/?path=/story/area-chart--with-negative-and-positive&knob-Y scale=log',
@@ -76,9 +74,7 @@ test.describe('Area series stories', () => {
 
     test('shows only negative values when hiding positive one', async ({ page }) => {
       const action = async () => {
-        await page.keyboard.down(await common.getModifierKey(page)());
         await page.click('.echLegendItem:nth-child(1) .echLegendItem__label');
-        await page.keyboard.up(await common.getModifierKey(page)());
       };
       await common.expectChartAtUrlToMatchScreenshot(page)(
         'http://localhost:9001/?path=/story/area-chart--with-negative-and-positive&knob-Y scale=log',

diff --git a/e2e/tests/interactions.test.ts b/e2e/tests/interactions.test.ts
@@ -347,7 +347,7 @@ test.describe('Interactions', () => {
             count: 2,
           },
           {
-            key: `${await common.getModifierKey(page)()}+Enter`,
+            key: 'enter',
             count: 1,
           },
         ],

diff --git a/e2e/tests/legend_stories.test.ts b/e2e/tests/legend_stories.test.ts
@@ -153,7 +153,7 @@ test.describe('Legend stories', () => {
             count: 2,
           },
           {
-            key: `${await common.getModifierKey(page)()}+Enter`,
+            key: 'enter',
             count: 1,
           },
         ],
@@ -174,7 +174,7 @@ test.describe('Legend stories', () => {
       // Make the first index legend item hidden
       await page.keyboard.press('Tab');
       await page.keyboard.press('Tab');
-      await page.keyboard.press(`${await common.getModifierKey(page)()}+Enter`);
+      await page.keyboard.press('Enter');
 
       const hiddenResults: number[] = [];
       // Filter the labels

diff --git a/e2e/tests/mixed_stories.test.ts b/e2e/tests/mixed_stories.test.ts
@@ -216,8 +216,6 @@ test.describe('Mixed series stories', () => {
 
               test('should show area chart with toggled series and mouse over', async ({ page }) => {
                 const action = async () => {
-                  // hold the meta/control key to hide rather than isolate
-                  await page.keyboard.down(await common.getModifierKey(page)());
                   await page.click('.echLegendItem:nth-child(2) .echLegendItem__label');
                 };
                 await common.expectChartWithMouseAtUrlToMatchScreenshot(page)(

diff --git a/packages/charts/src/chart_types/xy_chart/legend/legend.test.ts b/packages/charts/src/chart_types/xy_chart/legend/legend.test.ts
@@ -78,31 +78,6 @@ describe('Legends', () => {
   beforeEach(() => {
     store = MockStore.default();
   });
-
-  function addBarSeries(n: number) {
-    const colors = ['red', 'blue', 'green', 'violet', 'orange', 'yellow', 'brown', 'black', 'white', 'gray'];
-    MockStore.addSpecs(
-      [
-        ...Array.from({ length: n }, (_, i) =>
-          MockSeriesSpec.bar({
-            id: `spec${i + 1}`,
-            data: [
-              {
-                x: 0,
-                y: 1,
-              },
-            ],
-          }),
-        ),
-        MockGlobalSpec.settings({
-          showLegend: true,
-          theme: { colors: { vizColors: colors.slice(0, n) } },
-        }),
-      ],
-      store,
-    );
-  }
-
   it('compute legend for a single series', () => {
     MockStore.addSpecs(
       [
@@ -254,72 +229,73 @@ describe('Legends', () => {
   });
 
   it('default all series legend items to visible when deselectedDataSeries is null', () => {
-    addBarSeries(3);
+    MockStore.addSpecs(
+      [
+        MockSeriesSpec.bar({
+          id: 'spec1',
+          data: [
+            {
+              x: 0,
+              y: 1,
+            },
+          ],
+        }),
+        MockSeriesSpec.bar({
+          id: 'spec2',
+          data: [
+            {
+              x: 0,
+              y: 1,
+            },
+          ],
+        }),
+        MockGlobalSpec.settings({
+          showLegend: true,
+          theme: { colors: { vizColors: ['red', 'blue'] } },
+        }),
+      ],
+      store,
+    );
     const legend = computeLegendSelector(store.getState());
 
     const visibility = legend.map((item) => !item.isSeriesHidden);
 
-    expect(visibility).toEqual([true, true, true]);
+    expect(visibility).toEqual([true, true]);
   });
   it('selectively sets series to visible when there are deselectedDataSeries items', () => {
-    addBarSeries(3);
+    MockStore.addSpecs(
+      [
+        MockSeriesSpec.bar({
+          id: 'spec1',
+          data: [
+            {
+              x: 0,
+              y: 1,
+            },
+          ],
+        }),
+        MockSeriesSpec.bar({
+          id: 'spec2',
+          data: [
+            {
+              x: 0,
+              y: 1,
+            },
+          ],
+        }),
+        MockGlobalSpec.settings({
+          showLegend: true,
+          theme: { colors: { vizColors: ['red', 'blue'] } },
+        }),
+      ],
+      store,
+    );
     const { key, specId } = computeSeriesDomainsSelector(store.getState()).formattedDataSeries[0]!;
 
     store.dispatch(onToggleDeselectSeriesAction([{ key, specId }]));
     const legend = computeLegendSelector(store.getState());
     const visibility = legend.map((item) => !item.isSeriesHidden);
-    // only the clicked item should be visible
-    expect(visibility).toEqual([true, false, false]);
-  });
-  it('resets all series to be visible when clicking again the only visible item', () => {
-    addBarSeries(3);
-    const { key, specId } = computeSeriesDomainsSelector(store.getState()).formattedDataSeries[0]!;
-    // click the first item
-    store.dispatch(onToggleDeselectSeriesAction([{ key, specId }]));
-    // now click again the same item
-    store.dispatch(onToggleDeselectSeriesAction([{ key, specId }]));
-    const legend = computeLegendSelector(store.getState());
-    const visibility = legend.map((item) => !item.isSeriesHidden);
-    expect(visibility).toEqual([true, true, true]);
-  });
-  it('makes it visible when a hidden series is clicked', () => {
-    addBarSeries(3);
-    const { key, specId } = computeSeriesDomainsSelector(store.getState()).formattedDataSeries[0]!;
-    // click the first item
-    store.dispatch(onToggleDeselectSeriesAction([{ key, specId }]));
-    const { key: otherKey, specId: otherSpecId } = computeSeriesDomainsSelector(store.getState())
-      .formattedDataSeries[1]!;
-    // now click the second item (now hidden)
-    store.dispatch(onToggleDeselectSeriesAction([{ key: otherKey, specId: otherSpecId }]));
-    const legend = computeLegendSelector(store.getState());
-    const visibility = legend.map((item) => !item.isSeriesHidden);
-    expect(visibility).toEqual([true, true, false]);
-  });
-  it('makes it hidden the clicked series if there are more than one series visible', () => {
-    addBarSeries(3);
-    const { key, specId } = computeSeriesDomainsSelector(store.getState()).formattedDataSeries[0]!;
-    // click the first item
-    store.dispatch(onToggleDeselectSeriesAction([{ key, specId }]));
-    const { key: otherKey, specId: otherSpecId } = computeSeriesDomainsSelector(store.getState())
-      .formattedDataSeries[1]!;
-    // now click the second item (now hidden)
-    store.dispatch(onToggleDeselectSeriesAction([{ key: otherKey, specId: otherSpecId }]));
-    // ...and click again this second item to make it hidden
-    store.dispatch(onToggleDeselectSeriesAction([{ key: otherKey, specId: otherSpecId }]));
-    const legend = computeLegendSelector(store.getState());
-    const visibility = legend.map((item) => !item.isSeriesHidden);
-    expect(visibility).toEqual([true, false, false]);
-  });
-  it('make it possible to hide all series using meta key on the only visible item', () => {
-    addBarSeries(3);
-    const { key, specId } = computeSeriesDomainsSelector(store.getState()).formattedDataSeries[0]!;
-    // click the first item
-    store.dispatch(onToggleDeselectSeriesAction([{ key, specId }]));
-    // now click again with the meta key enabled
-    store.dispatch(onToggleDeselectSeriesAction([{ key, specId }], true));
-    const legend = computeLegendSelector(store.getState());
-    const visibility = legend.map((item) => !item.isSeriesHidden);
-    expect(visibility).toEqual([false, false, false]);
+    expect(visibility).toEqual([false, true]);
   });
   it('returns the right series name for a color series', () => {
     const seriesIdentifier1 = {

diff --git a/packages/charts/src/components/legend/label.tsx b/packages/charts/src/components/legend/label.tsx
@@ -20,8 +20,6 @@ interface LabelProps {
   options: LegendLabelOptions;
 }
 
-const isAppleDevice = typeof window !== 'undefined' && /Mac|iPhone|iPad/.test(window.navigator.userAgent);
-
 /**
  * Label component used to display text in legend item
  * @internal
@@ -34,13 +32,10 @@ export function Label({ label, isToggleable, onToggle, isSeriesHidden, options }
     'echLegendItem__label--multiline': maxLines > 1,
   });
 
-  const onClick: MouseEventHandler = useCallback(
-    ({ metaKey, ctrlKey }) => onToggle?.(isAppleDevice ? metaKey : ctrlKey),
-    [onToggle],
-  );
+  const onClick: MouseEventHandler = useCallback(({ shiftKey }) => onToggle?.(shiftKey), [onToggle]);
   const onKeyDown: KeyboardEventHandler = useCallback(
-    ({ key, metaKey, ctrlKey }) => {
-      if (key === ' ' || key === 'Enter') onToggle?.(isAppleDevice ? metaKey : ctrlKey);
+    ({ key, shiftKey }) => {
+      if (key === ' ' || key === 'Enter') onToggle?.(shiftKey);
     },
     [onToggle],
   );

diff --git a/packages/charts/src/state/actions/legend.ts b/packages/charts/src/state/actions/legend.ts
@@ -47,7 +47,7 @@ interface LegendItemOutAction {
 export interface ToggleDeselectSeriesAction {
   type: typeof ON_TOGGLE_DESELECT_SERIES;
   legendItemIds: SeriesIdentifier[];
-  metaKey: boolean;
+  negate: boolean;
 }
 
 /** @internal */
@@ -63,9 +63,9 @@ export function onLegendItemOutAction(): LegendItemOutAction {
 /** @internal */
 export function onToggleDeselectSeriesAction(
   legendItemIds: SeriesIdentifier[],
-  metaKey = false,
+  negate = false,
 ): ToggleDeselectSeriesAction {
-  return { type: ON_TOGGLE_DESELECT_SERIES, legendItemIds, metaKey };
+  return { type: ON_TOGGLE_DESELECT_SERIES, legendItemIds, negate };
 }
 
 /** @internal */

diff --git a/packages/charts/src/state/reducers/interactions.ts b/packages/charts/src/state/reducers/interactions.ts
@@ -264,7 +264,7 @@ export function interactionsReducer(
  */
 
 function toggleDeselectedDataSeries(
-  { legendItemIds, metaKey }: ToggleDeselectSeriesAction,
+  { legendItemIds, negate }: ToggleDeselectSeriesAction,
   deselectedDataSeries: SeriesIdentifier[],
   legendItems: LegendItem[],
 ) {
@@ -273,28 +273,19 @@ function toggleDeselectedDataSeries(
   const legendItemsKeys = legendItems.map(({ seriesIdentifiers }) => seriesIdentifiers);
 
   const alreadyDeselected = actionSeriesKeys.every((key) => deselectedDataSeriesKeys.has(key));
-  const keepOnlyNonActionSeries = ({ key }: SeriesIdentifier) => !actionSeriesKeys.includes(key);
 
-  // when a meta key (CTRL or Mac Cmd ⌘) add or remove the clicked item from the visible list
-  if (metaKey) {
+  // todo consider branch simplifications
+  if (negate) {
+    return alreadyDeselected || deselectedDataSeries.length !== legendItemsKeys.length - 1
+      ? legendItems
+          .flatMap(({ seriesIdentifiers }) => seriesIdentifiers)
+          .filter(({ key }) => !actionSeriesKeys.includes(key))
+      : legendItemIds;
+  } else {
     return alreadyDeselected
-      ? deselectedDataSeries.filter(keepOnlyNonActionSeries)
-      : deselectedDataSeries.concat(legendItemIds);
+      ? deselectedDataSeries.filter(({ key }) => !actionSeriesKeys.includes(key))
+      : [...deselectedDataSeries, ...legendItemIds];
   }
-  // when a hidden series is clicked, make it visible
-  if (alreadyDeselected) {
-    return deselectedDataSeries.filter(keepOnlyNonActionSeries);
-  }
-  // if the clicked item is the only visible series, make all series visible (reset)
-  if (deselectedDataSeries.length === legendItemsKeys.length - 1) {
-    return [];
-  }
-  // at this point either a visible series was clicked:
-  // * if there's at least one hidden series => add it to the hidden list
-  // * otherwise hide everything but the clicked item (isolate it)
-  return deselectedDataSeries.length
-    ? deselectedDataSeries.concat(legendItemIds)
-    : legendItemsKeys.flat().filter(keepOnlyNonActionSeries);
 }
 
 function getDrilldownData(globalState: GlobalChartState) {