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.

Sign up for GitHub

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

fix: Allow popovers in charts to be dismissed by pressing Esc #514

Merged
merged 1 commit into from
Nov 25, 2022
Merged

fix: Allow popovers in charts to be dismissed by pressing Esc #514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 21 additions & 0 deletions src/area-chart/__integ__/area-chart.test.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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 => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional that this test navigates to '#/light/bar-chart/test' even though the test file name is mixed-line-bar-char-test.ts? If we are testing the Bar Chart, would it make more sense to move this test to the existing file bar-chart/__tests__/bar-chart.test.ts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. I understand that the component is common for both chart variants, but still a bit confusing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following the similar pattern to the rest of the page, which tests a mix of mixed-line-bar-chart, line-chart, and bar-chart components, presumably so that there's a good mix of tested components. This behavior is implemented on the mixed component, so technically testing it on the bar chart is the same (in my opinion!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, now it makes more sense :)

// 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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can also check, whether it's really open or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is covered that same way in integration tests for each place where it's implemented.

}
};
document.addEventListener('keydown', onKeyDown);
return () => document.removeEventListener('keydown', onKeyDown);
}, [clearHighlightedSegment]);

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