Skip to content

Commit

Permalink
fix: Allow popovers in charts to be dismissed by pressing Esc (#514)
Browse files Browse the repository at this point in the history
  • Loading branch information
avinashbot authored Nov 25, 2022
1 parent 218aa7e commit 4bb4f3a
Show file tree
Hide file tree
Showing 8 changed files with 114 additions and 7 deletions.
21 changes: 21 additions & 0 deletions src/area-chart/__integ__/area-chart.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,27 @@ describe('Popover', () => {
await expect(page.getPopoverTitle()).resolves.toBe('5s');
})
);

test(
'popover can be closed when Escape is pressed after hover',
setupTest('#/light/area-chart/test', 'Linear latency chart', async page => {
await expect(page.hasPopover()).resolves.toBe(false);
await page.hoverElement(page.chart.toSelector());
await expect(page.hasPopover()).resolves.toBe(true);
await page.keys(['Escape']);
await expect(page.hasPopover()).resolves.toBe(false);
})
);

test(
'popover can be closed when Escape is pressed after chart plot is focused',
setupTest('#/light/area-chart/test', 'Linear latency chart', async page => {
await page.focusPlot();
await expect(page.hasPopover()).resolves.toBe(true);
await page.keys(['Escape']);
await expect(page.hasPopover()).resolves.toBe(false);
})
);
});

describe('Keyboard navigation', () => {
Expand Down
6 changes: 6 additions & 0 deletions src/area-chart/internal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,12 @@ export default function InternalAreaChart<T extends AreaChartProps.DataTypes>({
const reserveLegendSpace = !showChart && !hideLegend;
const reserveFilterSpace = !showChart && !isNoMatch && (!hideFilter || additionalFilters);

useEffect(() => {
const onKeyDown = model.handlers.onDocumentKeyDown;
document.addEventListener('keydown', onKeyDown);
return () => document.removeEventListener('keydown', onKeyDown);
}, [model.handlers.onDocumentKeyDown]);

const onBlur = (event: React.FocusEvent) => {
if (event.relatedTarget && !nodeContains(containerRef.current, event.relatedTarget)) {
model.handlers.onContainerBlur();
Expand Down
1 change: 1 addition & 0 deletions src/area-chart/model/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export interface ChartModel<T extends AreaChartProps.DataTypes> {
onLegendHighlight: (series: null | AreaChartProps.Series<T>) => void;
onPopoverDismiss: (outsideClick?: boolean) => void;
onContainerBlur: () => void;
onDocumentKeyDown: (event: KeyboardEvent) => void;
};
interactions: ReadonlyAsyncStore<ChartModel.InteractionsState<T>>;
refs: {
Expand Down
8 changes: 8 additions & 0 deletions src/area-chart/model/use-chart-model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,13 @@ export default function useChartModel<T extends AreaChartProps.DataTypes>({
interactions.clearState();
};

const onDocumentKeyDown = (event: KeyboardEvent) => {
if (event.key === 'Escape') {
interactions.clearHighlight();
interactions.clearHighlightedLegend();
}
};

return {
width,
height,
Expand All @@ -260,6 +267,7 @@ export default function useChartModel<T extends AreaChartProps.DataTypes>({
onLegendHighlight,
onPopoverDismiss,
onContainerBlur,
onDocumentKeyDown,
},
refs: {
plot: plotRef,
Expand Down
29 changes: 29 additions & 0 deletions src/mixed-line-bar-chart/__integ__/mixed-line-bar-chart.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,35 @@ describe('Details popover', () => {
})
);

test(
'can be hidden after hover by pressing Escape',
setupTest('#/light/mixed-line-bar-chart/test', async page => {
// Hover over first group
await page.hoverElement(chartWrapper.findBarGroups().get(1).toSelector());
await expect(page.getText(popoverHeaderSelector())).resolves.toContain('Potatoes');

// Pressing escape should hide the popover
await page.keys(['Escape']);
await expect(page.isDisplayed(popoverDismissSelector())).resolves.toBe(false);
await expect(page.isDisplayed(popoverHeaderSelector())).resolves.toBe(false);
})
);

test(
'can be hidden after keyboard navigation by pressing Escape',
setupTest('#/light/bar-chart/test', async page => {
// Navigate first group in the first chart
await page.click('#focus-target');
await page.keys(['Tab', 'Tab', 'ArrowRight']);
await expect(page.getText(popoverHeaderSelector())).resolves.toContain('Potatoes');

// Pressing Escape should close the popover
await page.keys(['Escape']);
await expect(page.isDisplayed(popoverHeaderSelector())).resolves.toBe(false);
await expect(page.isDisplayed(popoverDismissSelector())).resolves.toBe(false);
})
);

test(
'can be pinned by clicking on chart background and dismissed by clicking outside chart area in line chart',
setupTest('#/light/line-chart/test', async page => {
Expand Down
22 changes: 16 additions & 6 deletions src/mixed-line-bar-chart/chart-container.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,22 @@ export default function ChartContainer<T extends ChartDataTypes>({
return null;
}, [highlightedPoint, verticalMarkerLeft, highlightedGroupIndex, scaledSeries, barGroups]);

useEffect(() => {
const onKeyDown = (event: KeyboardEvent) => {
if (event.key === 'Escape') {
dismissPopover();
}
};
document.addEventListener('keydown', onKeyDown);
return () => document.removeEventListener('keydown', onKeyDown);
}, [dismissPopover]);

useLayoutEffect(() => {
if (highlightedX !== null || highlightedPoint !== null) {
showPopover();
}
}, [highlightedX, highlightedPoint, showPopover]);

const onPopoverDismiss = (outsideClick?: boolean) => {
dismissPopover();

Expand All @@ -278,12 +294,6 @@ export default function ChartContainer<T extends ChartDataTypes>({
}
};

useLayoutEffect(() => {
if (highlightedX !== null) {
showPopover();
}
}, [highlightedX, showPopover]);

const onSVGMouseDown = (e: React.MouseEvent<SVGSVGElement, MouseEvent>) => {
if (isPopoverOpen) {
if (isPopoverPinned) {
Expand Down
21 changes: 21 additions & 0 deletions src/pie-chart/__integ__/pie-chart.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,27 @@ describe('Detail popover', () => {
await expect(page.isDisplayed(detailsPopoverSelector)).resolves.toBe(false);
})
);

test(
'can be dismissed after hovering on the segment by pressing Escape',
setupTest(async page => {
await page.hoverElement(pieWrapper.findSegments().get(1).toSelector());
await page.waitForVisible(detailsPopoverSelector);
await page.keys(['Escape']);
await expect(page.isDisplayed(detailsPopoverSelector)).resolves.toBe(false);
})
);

test(
'can be dismissed after navigating to the segment with keyboard by pressing Escape',
setupTest(async page => {
await page.click('#focus-target');
await page.keys(['Tab', 'Tab', 'Enter']);
await page.waitForVisible(detailsPopoverSelector);
await page.keys(['Escape']);
await expect(page.isDisplayed(detailsPopoverSelector)).resolves.toBe(false);
})
);
});

describe('Focus outline', () => {
Expand Down
13 changes: 12 additions & 1 deletion src/pie-chart/pie-chart.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
import React, { useCallback, useMemo, useRef, useState } from 'react';
import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react';
import clsx from 'clsx';
import { pie } from 'd3-shape';

Expand Down Expand Up @@ -164,11 +164,22 @@ export default <T extends PieChartProps.Datum>({
},
[highlightedSegment, setTooltipOpen, onHighlightChange]
);

const clearHighlightedSegment = useCallback(() => {
setTooltipOpen(false);
onHighlightChange(null);
}, [onHighlightChange, setTooltipOpen]);

useEffect(() => {
const onKeyDown = (event: KeyboardEvent) => {
if (event.key === 'Escape') {
clearHighlightedSegment();
}
};
document.addEventListener('keydown', onKeyDown);
return () => document.removeEventListener('keydown', onKeyDown);
}, [clearHighlightedSegment]);

const onMouseDown = useCallback(
(internalDatum: InternalChartDatum<T>) => {
if (pinnedSegment === internalDatum.datum) {
Expand Down

0 comments on commit 4bb4f3a

Please sign in to comment.