Announce file selected (as in included in commit) state #16420
Conversation
The `focusOnHover` only comes into play when `selectOnHover` is set and the only component using that (now) is the `AutoCompletingTextInput`. Due to a bug in the mouseOver event handler on the List component we would only invoke the `selectionChanged` event and not the `selectedRowChanged` event meaning that hovering wouldn't ever change the selection. The only thing it would do is scroll to partially visible rows which isn't a common behavior and not worth keeping all this logic (and a11y lint exception) around for.
app/src/ui/lib/list/list.tsx
Outdated
| /** The aria-labelledby attribute for the list component. */ | ||
| readonly 'aria-labelledby'?: string | ||
|
|
||
| /** The aria-label attribute for the list component. */ | ||
| readonly 'aria-label'?: string |
There was a problem hiding this comment.
Seems like this could be a use case for the new AccessibilityProps?
I am thinking that we should make AccessibilityProps be the preferred way to add aria attributes so we have consistent approach. With this PR, we now have three approaches ariaLabel (camel casing props), aria-label (quoting props), and using the AccessibilityProps. Accessibility props does give us an easy range of inclusion of any aria-label we want... only downside I see is that it almost indicates that any of those aria-props are valid, but some like aria-checked only make sense on things like checkboxes and radio buttons. :/
If we don't use the AccessibilityProps, I prefer using the camel casing on react components as it makes it clear that it is a react prop not an aria attribute implementation which you can add to react component whether or not there is a prop to receive it.
There was a problem hiding this comment.
I looked at polishing this up to a point where we could switch to using accessibility props all over but there are nuances here around ensuring we don't accidentally pick props not applicable to the component and decided that this isn't worth spending time on right now.
xref: https://github.com/github/accessibility-audits/issues/3277
xref: https://github.com/github/accessibility-audits/issues/3261
Description
This ensure that we announce whether a file in the changes list will be included in the commit or not. Additionally this makes empty lists tab reachable such that we can let accessibility tech know that there are no changed files.
In this I added a way to add arbitrary aria attributes (and override existing) on list rows. This ended up not being necessary but I think the change has merit and included it anyway. The AccessibilityProps is a pattern I imaging we could use for many of our components as an easy way to let consumers set all of the available aria attributes without us having to manually add and map them. If need be I can remove that change from this PR and offer it up in another but it'd be nice to not have to do that.
Screenshots
Release notes
Notes: