fix(button): add aria labels for accessibility #3377
Conversation
src/components/button/Button.tsx
Outdated
| @@ -92,6 +95,7 @@ class Button extends React.Component<ButtonProps> { | |||
| let button = ( | |||
| // eslint-disable-next-line react/button-has-type | |||
| <button | |||
| aria-label={ariaLabel} | |||
There was a problem hiding this comment.
the Button component uses the rest functionality to allow additional HTML attributes. this is how other components have been adding aria labels:
are we able to continue using this pattern? any specific reason for the explicit prop now?
There was a problem hiding this comment.
Thank you for showing me examples. I made changes to use the rest functionality.
| /** Custom class for the close button */ | ||
| className?: string, | ||
| /** onClick handler for the close button */ | ||
| onClick?: Function, | ||
| } | ||
|
|
||
| const CloseButton = ({ className, onClick }: Props) => { | ||
| const CloseButton = ({ className, onClick, ariaLabel = 'close' }: Props) => { |
There was a problem hiding this comment.
the Close message should be translated since this is read by screen readers
There was a problem hiding this comment.
I've removed the hardcoded value and I used the IntlShape for close message translation. Thanks a lot!
| /** Custom class for the close button */ | ||
| className?: string, | ||
| /** onClick handler for the close button */ | ||
| onClick?: Function, | ||
| } | ||
|
|
||
| const CloseButton = ({ className, onClick }: Props) => { | ||
| const CloseButton = ({ className, onClick, ariaLabel = 'close' }: Props) => { |
There was a problem hiding this comment.
nit: these props are sorted alphabetically
There was a problem hiding this comment.
It's been removed. Thanks!
| /** Custom class for the close button */ | ||
| className?: string, | ||
| /** onClick handler for the close button */ | ||
| onClick?: Function, | ||
| } | ||
|
|
||
| const CloseButton = ({ className, onClick }: Props) => { | ||
| const CloseButton = ({ className, onClick, ariaLabel = 'close' }: Props) => { |
There was a problem hiding this comment.
any reason why ariaLabel is a prop in this component? i.e. is there a scenario in which a parent component would pass a different label than "Close"?
if not, then you could pass the attribute directly to Button without making a new prop in CloseButton
There was a problem hiding this comment.
I've found a proper close message for CloseButton in common/messages.js. Now the label is properly translated and I've removed aria-label from props.
| import './CloseButton.scss'; | ||
|
|
||
| export interface CloseButtonProps { | ||
| intl: IntlShape; |
There was a problem hiding this comment.
nit: this list is sorted alphabetically
| /** Custom class for the close button */ | ||
| className?: string; | ||
| /** onClick handler for the close button */ | ||
| onClick?: Function; | ||
| } | ||
|
|
||
| const CloseButton = ({ className, onClick }: CloseButtonProps) => { | ||
| const CloseButton = ({ intl, className, onClick }: CloseButtonProps) => { |
| @@ -37,4 +37,12 @@ describe('components/close-button/CloseButton', () => { | |||
| expect(handleClick).toHaveBeenCalledTimes(1); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('accesability properties', () => { | |||
| test('should have an accesiblity label', () => { | |||
There was a problem hiding this comment.
could we move this into the render() describe above? this test probably doesn't need to be in its own describe
| @@ -27,4 +34,5 @@ const CloseButton = ({ className, onClick }: CloseButtonProps) => { | |||
| ); | |||
| }; | |||
|
|
|||
| export default CloseButton; | |||
| export { CloseButton as CloseButtonBase }; | |||
There was a problem hiding this comment.
usually we do this export so that we can test the component without the injectIntl HOC. do we need to update the import in the test?
There was a problem hiding this comment.
I removed an unused export, leaving just the one with injectIntl.
dsporysz-box
force-pushed
the
button-accessability-label
branch
from
03b425f
to
f1a4459
Compare
Add an optional aria-label property to CloseButton and Button. Note that the "aria-label" property in CloseButton has a default value "close".