Skip to content

Commit

Permalink
fix: Click outside of a chart removes section selection (#592)
Browse files Browse the repository at this point in the history
  • Loading branch information
Al-Dani authored Dec 21, 2022
1 parent 1de0512 commit c5f6ca8
Show file tree
Hide file tree
Showing 6 changed files with 87 additions and 12 deletions.
16 changes: 16 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 @@ -4,6 +4,7 @@ import useBrowser from '@cloudscape-design/browser-test-tools/use-browser';
import { BasePageObject } from '@cloudscape-design/browser-test-tools/page-objects';
import createWrapper, { BarChartWrapper, MixedLineBarChartWrapper } from '../../../lib/components/test-utils/selectors';
import chartPlotStyles from '../../../lib/components/internal/components/chart-plot/styles.selectors.js';
import mixedChartStyles from '../../../lib/components/mixed-line-bar-chart/styles.selectors.js';

const chartWrapper = createWrapper().findMixedLineBarChart('#chart');
const groupedBarWrapper = new BarChartWrapper('#chart-grouped');
Expand All @@ -21,6 +22,8 @@ const highlightedSeriesSelector = (wrapper: MixedLineBarChartWrapper = chartWrap
wrapper.findHighlightedSeries().toSelector();
const seriesSVGSelector = (wrapper: MixedLineBarChartWrapper = chartWrapper) =>
wrapper.find(`.${chartPlotStyles.root}`).toSelector();
const dimmedElementsSelector = (wrapper: MixedLineBarChartWrapper = chartWrapper) =>
wrapper.findAll(`.${mixedChartStyles['series--dimmed']}`).toSelector();
const filterWrapper = chartWrapper.findDefaultFilter();
const legendWrapper = chartWrapper.findLegend();

Expand Down Expand Up @@ -256,6 +259,19 @@ describe('Series', () => {
await expect(page.getText(popoverContentSelector())).resolves.toContain('Threshold\n420');
})
);

test(
'clicking outside of the chart removes all highlights for pinned element',
setupTest('#/light/bar-chart/test', async page => {
// Click on it to reveal the dismiss button
await page.hoverElement(chartWrapper.findBarGroups().get(3).toSelector());
await page.click(chartWrapper.toSelector());
await expect(page.isDisplayed(dimmedElementsSelector())).resolves.toBe(true);

await page.click('#focus-target');
await expect(page.isDisplayed(dimmedElementsSelector())).resolves.toBe(false);
})
);
});

describe('Details popover', () => {
Expand Down
12 changes: 12 additions & 0 deletions src/mixed-line-bar-chart/__tests__/mixed-chart.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -948,6 +948,18 @@ describe('Details popover', () => {

expect(wrapper.findApplication()!.getElement()).toHaveFocus();
});

test('no highlighted segment when pressing outside', () => {
const { wrapper } = renderMixedChart(<MixedLineBarChart {...barChartProps} />);

wrapper.findApplication()!.focus();
wrapper.findChart()!.fireEvent(new MouseEvent('mousedown', { bubbles: true }));

expect(wrapper.findByClassName(styles['series--dimmed'])).not.toBeNull();

wrapper.findDefaultFilter()?.openDropdown();
expect(wrapper.findByClassName(styles['series--dimmed'])).toBeNull();
});
});

test('highlighted series are controllable', () => {
Expand Down
21 changes: 13 additions & 8 deletions src/mixed-line-bar-chart/chart-container.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -202,17 +202,21 @@ export default function ChartContainer<T extends ChartDataTypes>({
[setHighlightedGroupIndex, setHighlightedPoint, highlightSeries]
);

const clearAllHighlights = useCallback(() => {
setHighlightedPoint(null);
highlightSeries(null);
setHighlightedGroupIndex(null);
}, [highlightSeries, setHighlightedGroupIndex, setHighlightedPoint]);

// Highlight all points at a given X in a line chart
const highlightX = useCallback(
(marker: VerticalMarkerX<T> | null) => {
if (marker) {
setHighlightedPoint(null);
highlightSeries(null);
setHighlightedGroupIndex(null);
clearAllHighlights();
}
setVerticalMarkerX(marker);
},
[highlightSeries, setHighlightedGroupIndex, setHighlightedPoint]
[clearAllHighlights]
);

// Highlight all points and bars at a given X index in a mixed line and bar chart
Expand All @@ -226,11 +230,9 @@ export default function ChartContainer<T extends ChartDataTypes>({
);

const clearHighlightedSeries = useCallback(() => {
highlightSeries(null);
setHighlightedGroupIndex(null);
setHighlightedPoint(null);
clearAllHighlights();
dismissPopover();
}, [dismissPopover, highlightSeries, setHighlightedGroupIndex, setHighlightedPoint]);
}, [dismissPopover, clearAllHighlights]);

const { isGroupNavigation, ...handlers } = useNavigation({
series,
Expand Down Expand Up @@ -306,6 +308,9 @@ export default function ChartContainer<T extends ChartDataTypes>({
plotRef.current?.focusPlot();
}
}, 0);
} else {
clearAllHighlights();
setVerticalMarkerX(null);
}
};

Expand Down
13 changes: 13 additions & 0 deletions src/pie-chart/__integ__/pie-chart.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,19 @@ describe('Segments', () => {
await expect(page.isDisplayed(highlightedSegmentSelector)).resolves.toBe(false);
})
);

test(
'clicking outside of the chart removes all highlights for pinned element',
setupTest(async page => {
await page.click(pieWrapper.findSegments().get(2).toSelector());
await expect(page.getText(pieWrapper.findHighlightedSegmentLabel().toSelector())).resolves.toContain('Chocolate');
await page.waitForVisible(detailsPopoverSelector);
await expect(page.isDisplayed(detailsDismissSelector)).resolves.toBe(true);

await page.click('#focus-target');
await expect(page.isDisplayed(highlightedSegmentSelector)).resolves.toBe(false);
})
);
});

describe('Filter', () => {
Expand Down
21 changes: 21 additions & 0 deletions src/pie-chart/__tests__/pie-chart.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,17 @@ describe('Segments', () => {
const { wrapper } = renderPieChart(<PieChart data={dataWithZero} highlightedSegment={dataWithZero[2]} />);
expect(wrapper.findHighlightedSegment()).toBeNull();
});

test('no highlighted segment when pressing outside', () => {
const { wrapper } = renderPieChart(<PieChart data={defaultData} />);
act(() => {
fireEvent.mouseDown(wrapper!.findSegments()[0].getElement());
});
expect(wrapper.findHighlightedSegment()).not.toBeNull();

wrapper.findDefaultFilter()?.openDropdown();
expect(wrapper.findHighlightedSegment()).toBeNull();
});
});

describe('Details popover', () => {
Expand Down Expand Up @@ -523,6 +534,16 @@ describe('Details popover', () => {
expect(wrapper.findDetailPopover()).toBeNull();
});

test('pressing spase on a focused segment opens the popover', () => {
const { wrapper } = renderPieChart(<PieChart data={defaultData} />);
wrapper.findSegments()[1].click();
wrapper.findDetailPopover()?.findDismissButton()?.click();

wrapper.findApplication()!.keydown(KeyCode.space);

expect(createWrapper(wrapper.getElement()).findPopover()?.findDismissButton()).not.toBeNull();
});

test('details popover can be customized', () => {
const { wrapper } = renderPieChart(
<PieChart
Expand Down
16 changes: 12 additions & 4 deletions src/pie-chart/pie-chart.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -220,26 +220,32 @@ export default <T extends PieChartProps.Datum>({
);
const onKeyDown = useCallback(
(event: React.KeyboardEvent) => {
if (event.keyCode !== KeyCode.right && event.keyCode !== KeyCode.left && event.keyCode !== KeyCode.enter) {
const keyCode = event.keyCode;
if (
keyCode !== KeyCode.right &&
keyCode !== KeyCode.left &&
keyCode !== KeyCode.enter &&
keyCode !== KeyCode.space
) {
return;
}

event.preventDefault();

let nextIndex = highlightedSegmentIndex || 0;
const MAX = pieData.length - 1;
if (event.keyCode === KeyCode.right) {
if (keyCode === KeyCode.right) {
nextIndex++;
if (nextIndex > MAX) {
nextIndex = 0;
}
} else if (event.keyCode === KeyCode.left) {
} else if (keyCode === KeyCode.left) {
nextIndex--;
if (nextIndex < 0) {
nextIndex = MAX;
}
}
if (event.keyCode === KeyCode.enter) {
if (keyCode === KeyCode.enter || keyCode === KeyCode.space) {
setPinnedSegment(pieData[nextIndex].data.datum);
}
highlightSegment(pieData[nextIndex].data);
Expand Down Expand Up @@ -286,6 +292,8 @@ export default <T extends PieChartProps.Datum>({
plotRef.current!.focusApplication();
popoverDismissedRecently.current = false;
}, 0);
} else {
onHighlightChange(null);
}
};

Expand Down

0 comments on commit c5f6ca8

Please sign in to comment.