fix: Fix accessibility issues #3095
Conversation
There was a problem hiding this comment.
Let me know if these changes make sense to you, @kresimir-coko
| */ | ||
| disabled?: boolean; | ||
| dropDownTriggerProps?: React.HTMLAttributes<HTMLButtonElement>; |
There was a problem hiding this comment.
Rather than have just aria labels available to the trigger element, I thought it would be better to provide a more robust API to allow more customization on that element. This allows them to add other accessibility attrs as needed.
There was a problem hiding this comment.
I see, you went a step further and added props for those triggers, nice!
| @@ -56,8 +56,8 @@ interface IProps extends React.HTMLAttributes<HTMLDivElement> { | |||
| * Labels for the aria attributes | |||
| */ | |||
| ariaLabels?: { | |||
| transferButtonLTR: string; | |||
| transferButtonRTL: string; | |||
| transferLTR: string; | |||
There was a problem hiding this comment.
For all ariaLabel props, I think we should focus less on the element itself and more on the language the user wants to use. So by removing Button from the name, it's more clear that this prop is for a language map.
There was a problem hiding this comment.
Yea the naming was a bit tricky to decide on
packages/clay-form/src/SelectBox.tsx
Outdated
| @@ -164,9 +156,9 @@ const ClaySelectBox: React.FunctionComponent<IProps> = ({ | |||
| ); | |||
|
|
|||
| return ( | |||
| <div {...otherProps} className={classNames(className, 'form-group')}> | |||
There was a problem hiding this comment.
Since this is called a Select component, I would expect the ...otherProps to apply to the select element and not the container. If we need the ability to add props to the container, we can use containerProps. This is really only for the case when the component name is not consistent with the container element.
There was a problem hiding this comment.
Alright, good to know! Makes sense now that you put it like that but I kinda forgot about containerProps
|
@bryceosterhaus all makes sense, the naming I was unsure of, as well as the |
Pull Request Test Coverage Report for Build 4674
💛 - Coveralls |
bryceosterhaus
changed the title
|
I squashed everything into one commit for the sake of versioning. Just need to add some updated docs for this |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 5e09db0:
|
Hey @kresimir-coko, here are some changes I made on top of your PR. Check out my PR comments for explanations of changes