Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
May 16, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 14 additions & 15 deletions e2e/page_objects/common.ts
Original file line number Diff line number Diff line change
@@ -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++;
}
}
};

4 changes: 0 additions & 4 deletions e2e/tests/area_stories.test.ts
Original file line number Diff line number Diff line change
@@ -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',
2 changes: 1 addition & 1 deletion e2e/tests/interactions.test.ts
Original file line number Diff line number Diff line change
@@ -347,7 +347,7 @@ test.describe('Interactions', () => {
count: 2,
},
{
key: `${await common.getModifierKey(page)()}+Enter`,
key: 'enter',
count: 1,
},
],
4 changes: 2 additions & 2 deletions e2e/tests/legend_stories.test.ts
Original file line number Diff line number Diff line change
@@ -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
2 changes: 0 additions & 2 deletions e2e/tests/mixed_stories.test.ts
Original file line number Diff line number Diff line change
@@ -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)(
136 changes: 56 additions & 80 deletions packages/charts/src/chart_types/xy_chart/legend/legend.test.ts
Original file line number Diff line number Diff line change
@@ -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 = {
11 changes: 3 additions & 8 deletions packages/charts/src/components/legend/label.tsx
Original file line number Diff line number Diff line change
@@ -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],
);
6 changes: 3 additions & 3 deletions packages/charts/src/state/actions/legend.ts
Original file line number Diff line number Diff line change
@@ -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 */
31 changes: 11 additions & 20 deletions packages/charts/src/state/reducers/interactions.ts
Original file line number Diff line number Diff line change
@@ -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) {