feat: Add filtering to button dropdown#4637
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4637 +/- ##
==========================================
+ Coverage 97.57% 97.60% +0.02%
==========================================
Files 948 950 +2
Lines 30489 30612 +123
Branches 11148 11216 +68
==========================================
+ Hits 29749 29878 +129
+ Misses 733 688 -45
- Partials 7 46 +39 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0d5d891 to
14e5718
Compare
14e5718 to
b0fd947
Compare
042705d to
50e2c55
Compare
50e2c55 to
d5f7211
Compare
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| import { ButtonDropdownProps } from '../interfaces'; | ||
| import { filterItems } from '../utils/filter-items'; |
There was a problem hiding this comment.
All these tests are technically possible to run from the component level: render button dropdown with certain items, set filtering - check the actual rendered items. Should we do this instead of testing the helper directly? This can give us better real coverage, as it not only ensures that the helper works, but also that it is used correctly by the component.
| expect(elementWrapper).toHaveTextContent('Custom'); | ||
| }); | ||
|
|
||
| test('renders custom item content for link items', () => { |
There was a problem hiding this comment.
Why did we add this test? Is that also the expected behaviour? I find it strange that we still render a link when the item renderer is overridden.
| }; | ||
|
|
||
| const actOnParentDropdown = (event: React.KeyboardEvent) => { | ||
| // if there is no highlighted item we act on the trigger by opening or closing dropdown |
There was a problem hiding this comment.
Did the behavior change here? or we don't need the comment?
| const activate = (event: React.KeyboardEvent, isEnter?: boolean) => { | ||
| setIsUsingMouse(false); | ||
|
|
||
| // if item is a link we rely on default behavior of an anchor, no need to prevent |
There was a problem hiding this comment.
Same, Did the behavior change here? or we don't need the comment?
| }); | ||
|
|
||
| const { isOpen, closeDropdown, ...openStateProps } = useOpenState({ onClose: reset }); | ||
| const prevFilteringValue = useRef(filteringValue); |
There was a problem hiding this comment.
Should we use usePrevious here?
const prevFilteringValue = usePrevious(filteringValue);
e553244 to
9fb88b8
Compare
9fb88b8 to
71b731a
Compare
71b731a to
3371c37
Compare
3371c37 to
7fe6f1d
Compare
7fe6f1d to
41d00ba
Compare
41d00ba to
17f2654
Compare
17f2654 to
f0e34d7
Compare
Description
Button dropdown action filtering implemented a similar way to select/multiselect filtering. The big underlying change is that filtering requires the use of
aria-activedescendantrather than simply moving focus from item to item. So most of the internal changes are about disabling focusing logic when filtering is active, so that the real focus can stay on the input (and "accessibility" focus can move around usingaria-activedescendant).Rel: AWSUI-61960 (Chorus: qodG8KXdJXXk)
How has this been tested?
Updated unit tests, a bit of manual accessibility testing, but more validation with other screen readers is needed.
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.