feat(Legend): change click on item behaviour (#2427)

* ✨ New click behaviour + CMD as new meta key * ✅ Add tests * test(vrt): update screenshots [skip ci] * ✅ Fix test * ✅ fix id * 👌 Revisit comments * 🐛 Add os specific detection * ✅ fix test * ♻️ Restore test * ✨ Add new command for playwright * 📸 Update screenshots * Revert "📸 Update screenshots" This reverts commit 332c895. * ✅ Move os detection * ✅ Move the check back * ✅ fix with new behaviour * ✅ Fix tests with new behaviour * ♻️ Refactor for TS * ✅ Fix tests * test(vrt): update screenshots [skip ci] * ✅ Fix more tests * test(vrt): update screenshots [skip ci] * :white_check_make: Refactor tests * test(vrt): update screenshots [skip ci] --------- Co-authored-by: elastic-datavis[bot] <98618603+elastic-datavis[bot]@users.noreply.github.com>
elastic · May 14, 2024 · b1c72df · b1c72df
1 parent 85811e5
commit b1c72df
Show file tree

Hide file tree

Showing 9 changed files with 135 additions and 90 deletions.
diff --git a/e2e/page_objects/common.ts b/e2e/page_objects/common.ts
@@ -44,10 +44,10 @@ interface ElementBBox {
   height: number;
 }
 
-interface KeyboardKey {
+type KeyboardKey = {
   key: string;
   count: number;
-}
+};
 
 interface ClickOptions {
   /**
@@ -217,6 +217,13 @@ 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
@@ -351,18 +358,12 @@ export class CommonPage {
    */
   // eslint-disable-next-line class-methods-use-this
   pressKey = (page: Page) => async (key: string, count: number) => {
-    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++;
-      }
+    // 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++;
     }
   };
 

diff --git a/e2e/tests/area_stories.test.ts b/e2e/tests/area_stories.test.ts
@@ -64,7 +64,9 @@ 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',
@@ -74,7 +76,9 @@ 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: 'enter',
+            key: `${await common.getModifierKey(page)()}+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: 'enter',
+            key: `${await common.getModifierKey(page)()}+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('Enter');
+      await page.keyboard.press(`${await common.getModifierKey(page)()}+Enter`);
 
       const hiddenResults: number[] = [];
       // Filter the labels

diff --git a/e2e/tests/mixed_stories.test.ts b/e2e/tests/mixed_stories.test.ts
@@ -216,6 +216,8 @@ 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,6 +78,31 @@ 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(
       [
@@ -229,73 +254,72 @@ describe('Legends', () => {
   });
 
   it('default all series legend items to visible when deselectedDataSeries is null', () => {
-    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,
-    );
+    addBarSeries(3);
     const legend = computeLegendSelector(store.getState());
 
     const visibility = legend.map((item) => !item.isSeriesHidden);
 
-    expect(visibility).toEqual([true, true]);
+    expect(visibility).toEqual([true, true, true]);
   });
   it('selectively sets series to visible when there are deselectedDataSeries items', () => {
-    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,
-    );
+    addBarSeries(3);
     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);
-    expect(visibility).toEqual([false, true]);
+    // 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]);
   });
   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,6 +20,8 @@ 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
@@ -32,10 +34,13 @@ export function Label({ label, isToggleable, onToggle, isSeriesHidden, options }
     'echLegendItem__label--multiline': maxLines > 1,
   });
 
-  const onClick: MouseEventHandler = useCallback(({ shiftKey }) => onToggle?.(shiftKey), [onToggle]);
+  const onClick: MouseEventHandler = useCallback(
+    ({ metaKey, ctrlKey }) => onToggle?.(isAppleDevice ? metaKey : ctrlKey),
+    [onToggle],
+  );
   const onKeyDown: KeyboardEventHandler = useCallback(
-    ({ key, shiftKey }) => {
-      if (key === ' ' || key === 'Enter') onToggle?.(shiftKey);
+    ({ key, metaKey, ctrlKey }) => {
+      if (key === ' ' || key === 'Enter') onToggle?.(isAppleDevice ? metaKey : ctrlKey);
     },
     [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[];
-  negate: boolean;
+  metaKey: boolean;
 }
 
 /** @internal */
@@ -63,9 +63,9 @@ export function onLegendItemOutAction(): LegendItemOutAction {
 /** @internal */
 export function onToggleDeselectSeriesAction(
   legendItemIds: SeriesIdentifier[],
-  negate = false,
+  metaKey = false,
 ): ToggleDeselectSeriesAction {
-  return { type: ON_TOGGLE_DESELECT_SERIES, legendItemIds, negate };
+  return { type: ON_TOGGLE_DESELECT_SERIES, legendItemIds, metaKey };
 }
 
 /** @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, negate }: ToggleDeselectSeriesAction,
+  { legendItemIds, metaKey }: ToggleDeselectSeriesAction,
   deselectedDataSeries: SeriesIdentifier[],
   legendItems: LegendItem[],
 ) {
@@ -273,19 +273,28 @@ function toggleDeselectedDataSeries(
   const legendItemsKeys = legendItems.map(({ seriesIdentifiers }) => seriesIdentifiers);
 
   const alreadyDeselected = actionSeriesKeys.every((key) => deselectedDataSeriesKeys.has(key));
+  const keepOnlyNonActionSeries = ({ key }: SeriesIdentifier) => !actionSeriesKeys.includes(key);
 
-  // todo consider branch simplifications
-  if (negate) {
-    return alreadyDeselected || deselectedDataSeries.length !== legendItemsKeys.length - 1
-      ? legendItems
-          .flatMap(({ seriesIdentifiers }) => seriesIdentifiers)
-          .filter(({ key }) => !actionSeriesKeys.includes(key))
-      : legendItemIds;
-  } else {
+  // when a meta key (CTRL or Mac Cmd ⌘) add or remove the clicked item from the visible list
+  if (metaKey) {
     return alreadyDeselected
-      ? deselectedDataSeries.filter(({ key }) => !actionSeriesKeys.includes(key))
-      : [...deselectedDataSeries, ...legendItemIds];
+      ? deselectedDataSeries.filter(keepOnlyNonActionSeries)
+      : deselectedDataSeries.concat(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) {