From b1c72df8698e15230f5f55c38f1a94d337e51c04 Mon Sep 17 00:00:00 2001 From: Marco Liberati Date: Tue, 14 May 2024 16:54:06 +0200 Subject: [PATCH] feat(Legend): change click on item behaviour (#2427) * :sparkles: New click behaviour + CMD as new meta key * :white_check_mark: Add tests * test(vrt): update screenshots [skip ci] * :white_check_mark: Fix test * :white_check_mark: fix id * :ok_hand: Revisit comments * :bug: Add os specific detection * :white_check_mark: fix test * :recycle: Restore test * :sparkles: Add new command for playwright * :camera_flash: Update screenshots * Revert ":camera_flash: Update screenshots" This reverts commit 332c895c543727bc54ff8d5238c8972dc407f4b0. * :white_check_mark: Move os detection * :white_check_mark: Move the check back * :white_check_mark: fix with new behaviour * :white_check_mark: Fix tests with new behaviour * :recycle: Refactor for TS * :white_check_mark: Fix tests * test(vrt): update screenshots [skip ci] * :white_check_mark: 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> --- e2e/page_objects/common.ts | 29 ++-- e2e/tests/area_stories.test.ts | 4 + e2e/tests/interactions.test.ts | 2 +- e2e/tests/legend_stories.test.ts | 4 +- e2e/tests/mixed_stories.test.ts | 2 + .../xy_chart/legend/legend.test.ts | 136 ++++++++++-------- .../charts/src/components/legend/label.tsx | 11 +- packages/charts/src/state/actions/legend.ts | 6 +- .../charts/src/state/reducers/interactions.ts | 31 ++-- 9 files changed, 135 insertions(+), 90 deletions(-) diff --git a/e2e/page_objects/common.ts b/e2e/page_objects/common.ts index be0e70d3f1..ec8dcede3e 100644 --- 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 index b03bf99bae..78de7b665d 100644 --- 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 index 90a930339d..565c93817b 100644 --- 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 index f13707778d..a5ddee3a1d 100644 --- 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 index 861621a942..e90bd11a39 100644 --- 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 index 0b8f5ff001..6c0d8fc894 100644 --- 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 index dd61c54e94..7f22af3d90 100644 --- 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 index 11e17c5190..a70b8d56e4 100644 --- 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 index 2a78b20dd1..8ff987cb64 100644 --- 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) {