fix: Update accessibility on avatar in expandable component #1736
Conversation
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
| Component: ({altText, ...elemProps}: ExpandableAvatarProps, ref) => { | ||
| return <StyledAvatar altText={undefined} as={'div'} ref={ref} size={32} {...elemProps} />; | ||
| }, |
There was a problem hiding this comment.
Interesting thing: Avatar component isn't built by our createComponent util function, so this component has some limitation on as prop. For example, with other components we can easily modify element in styled function using as method:
const StyledActionBarText = styled(Box.as('span'))<StyledType>({
textDecoration: 'underline',
})
Avatar component doesn't have this possibility 🤔 Also, our create component functions have a lot of typescript validation under the hood, and they are not exists in Avatar component because of it.
Should we review this moment and refactor Avatar component (an other components, if they exist) to use util functions?
There was a problem hiding this comment.
Good catch Raisa! Yeah the fact that Avatar is not a createComponent is hindering us here :( We can refactor this now, I don't think it should be a breaking change but it would greatly help us. Let me write up an issue to track this and maybe we can refactor it!
There was a problem hiding this comment.
Here's the issue: #1738
There was a problem hiding this comment.
yeah, Avatar is a main component, so we should be careful with making changes in it.
it shouldn't be any change of functionality for users, just inner refactoring so we are probably good to do it.
|
Changes implemented in #1739. |
Summary
Per accessibility, we don't need an alt text on the Expandable Avatar because it is a decorative element. Also, we default the element to be a div because we can't have a button within a button.