Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Update accessibility on avatar in expandable component #1736

Closed
wants to merge 5 commits into from
Closed

fix: Update accessibility on avatar in expandable component #1736

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
00b824a
Merge branch 'prerelease/minor' of https://github.com/Workday/canvas-…
Jul 18, 2022
10135cf
Merge branch 'prerelease/minor' of https://github.com/Workday/canvas-…
Aug 11, 2022
30074b5
Merge branch 'prerelease/minor' of https://github.com/Workday/canvas-…
Aug 15, 2022
bd6aca2
Merge branch 'prerelease/minor' of https://github.com/Workday/canvas-…
Aug 18, 2022
ce5558a
fix: Clean up accessibility on Avatar in expandable
Aug 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions modules/labs-react/expandable/lib/ExpandableAvatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
/** @jsxRuntime classic */
/** @jsx jsx */
import {jsx} from '@emotion/react';

import React from 'react';

import {createComponent, ExtractProps, styled, StyledType} from '@workday/canvas-kit-react/common';
Expand All @@ -19,9 +15,12 @@ const StyledAvatar = styled(Avatar)<StyledType>({
flexShrink: 0,
});

// When the component is created, it needs to be a button element to match AvatarProps.
// Once Avatar becomes a `createComponent` we can default the element type to a `div`
// and the types should be properly extracted
export const ExpandableAvatar = createComponent('button')({
displayName: 'Expandable.Avatar',
Component: ({...elemProps}: ExpandableAvatarProps, ref, Element) => {
return <StyledAvatar altText={undefined} as={Element} ref={ref} size={32} {...elemProps} />;
Component: ({altText, ...elemProps}: ExpandableAvatarProps, ref) => {
return <StyledAvatar altText={undefined} as={'div'} ref={ref} size={32} {...elemProps} />;
},
Comment on lines +23 to 25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the issue: #1738

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});