fix(List, ListItem): Improve accessibility for List and migrate tests #949
Conversation
PavelKirvelTextkernel
force-pushed
the
ONEUI-364_Migrate_List_tests
branch
3 times, most recently
from
766b0ba to
eb58ab1
Compare
PavelKirvelTextkernel
force-pushed
the
ONEUI-364_Migrate_List_tests
branch
from
eb58ab1 to
42d76dc
Compare
PavelKirvelTextkernel
changed the title
src/components/SelectComponents/Autosuggest/__tests__/Autosuggest.spec.tsx
Outdated
Show resolved
Hide resolved
src/components/SelectComponents/Combobox/__tests__/Combobox.spec.tsx
Outdated
Show resolved
Hide resolved
|
Related to forgotten comment i haven't finished those tests yet |
|
|
||
| expect(mockOnClick).toBeCalledWith(2); | ||
| }); | ||
| it.skip('should not call onClick after navigating to the next highlighted item', async () => { |
There was a problem hiding this comment.
I'm a bit stuck with this tests case.
Here we click on element and then expect that Enter will not clicked or first click doesn't count ?
There was a problem hiding this comment.
I tried to fix it, but indeed took me quite a bit of time to figure out what these tests should do, and why they don't really work. While investigating, I ended up refactoring quite a bit if the file, adding some tests, and deleting others. I think the suit looks better now (but I'm biased 😝). Since there were so many changes, I thought it is easier to just push my changes instead of trying to describe them here. It is your PR, if you don't like my changes, you can revert them and apply only the parts you think make sense.
There was a problem hiding this comment.
I've checked it looks great
# Conflicts: # TESTING_LIBRARY_MIGRATION.md # src/components/Chip/__tests__/Chip.spec.tsx # src/components/Dropdown/__tests__/Dropdown.spec.tsx # src/components/FieldWrapper/__tests__/FieldWrapper.spec.tsx # src/components/SelectComponents/Autosuggest/__tests__/Autosuggest.spec.tsx # src/components/SelectComponents/Combobox/__tests__/Combobox.spec.tsx # src/components/SelectComponents/ComboboxMulti/__tests__/ComboboxMulti.spec.tsx
| onClick={onClick} | ||
| aria-selected={isHighlighted} | ||
| onKeyDown={onClick} |
There was a problem hiding this comment.
I think this isn't the best practice to call onClick for any key press. Normally it is only Enter that should trigger it. Can you make it a handle function instead, that calls the callback only in that case?
There was a problem hiding this comment.
I've pushed changes for this action could you please check it
There was a problem hiding this comment.
I had a quick look, and it looks ok. however, as mentioned in the other comment, I'm still looking into it. My concern is that a ListItem that doesn't have an onClick handles now still looks like an interactive element with onKeyDown and role=option. I'm trying to figure out what is the best way to go about it. Can you leave this PR for me for a bit until I figure these out?
eszthoff
changed the title
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. Use imperative, present tense in your commit description ("change", not "changed" or "changes") without uppercases or period (.) at the end.
Task link: https://textkernel.atlassian.net/browse/ONEUI-364