fix: Allow popovers in charts to be dismissed by pressing Esc #514
Conversation
Codecov ReportBase: 92.52% // Head: 92.50% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #514 +/- ##
==========================================
- Coverage 92.52% 92.50% -0.03%
==========================================
Files 567 567
Lines 16135 16155 +20
Branches 4414 4417 +3
==========================================
+ Hits 14929 14944 +15
- Misses 1126 1130 +4
- Partials 80 81 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
avinashbot
requested review from
a team,
Al-Dani and
jperals
and removed request for
jperals and
a team
|
|
||
| test( | ||
| 'can be hidden after keyboard navigation by pressing Escape', | ||
| setupTest('#/light/bar-chart/test', async page => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
good point. I understand that the component is common for both chart variants, but still a bit confusing
There was a problem hiding this comment.
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!)
| useEffect(() => { | ||
| const onKeyDown = (event: KeyboardEvent) => { | ||
| if (event.key === 'Escape') { | ||
| clearHighlightedSegment(); |
There was a problem hiding this comment.
maybe we can also check, whether it's really open or not.
There was a problem hiding this comment.
This is covered that same way in integration tests for each place where it's implemented.
Description
The accessibility issue in question is that the details popovers appear on hover or during keyboard navigation, but they obscure content when they appear. Assistive technology users need a way to be able to dismiss these popovers without needing to drag the cursor all the way out of the chart.
Related links, issue #, if available: AWSUI-19204, AWSUI-19231, AWSUI-19238, AWSUI-19254, four issues baby wooo let's goooo
How has this been tested?
Integration tests, but feel free to test it yourself manually too!
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.