v5 component refactor: Avatar

[HollyJoyPhillips]

Abstract

As part of the v5 Elements release, each component will be reviewed and refactored to ensure best practice and design system alignment

Specification

• Is new component or existing? Existing - called 'Avatar' in Elements. However, should be reviewed/adhere to variants in the DS e.g. circle, rectangle & square
• Is the component being re-named? No but should review variants and ensure they are aligned
• Is the component design signed off by product and design? Yes
• Guide to component structure & documentation in storybook
• Link to DS in Figma
• Link to Design Guidelines
• Link to Elements
• Accessibility requirements

Developer Checklist

• Styles alignment between Design System and Elements
• Check design tokens in Figma and implement CSS variable tokens if available for relevant component
• Align with accessibility standards / spec as per above
• If relevant, break down component into Styles Only and React component structures
• Ensure all variants of components are documented as appropriate
• Ensure unit test coverage is adequate for component
• Update documentation in MDX file as per guidelines
• Changelog updated to reflect a single beta version per component ideally

Release Checklist

• Approved PR merged to main
• Design & product review and feedback addressed by developer
• Beta release by product / engineering lead to next beta version

Additional Context or Information

[ksolanki7]

After discussing with the Design team, I am moving this ticket back to the "To Do" column, as there are still clarifications needed and updates required for the component. The ticket will not be ready for development until these items are resolved by the Design team. Please refrain from picking up this ticket until it has been cleared by the Design team.

[HollyJoyPhillips]

Thank you @ksolanki7, I've reached out to the design team.

[ksolanki7]

@nurm717123 This ticket is ready to be picked. Here is the dev ready figma link - Figma Link

[nurm717123]

Context:

• there is three variants of avatar which one of them have different style than other that require more discussion (e.g rectangle avatar could accept image and have completely different style than circle and square avatar)
• Three of them will require too much code for a single PR.
• Avatar circle and square looks like have same style, the only different is its shape

Proposal

given the context above I propose to update 2 components out of it to complete this ticket:

1. Avatar (shape=circle/square, variants=letter/icon, src=string/IconName, size=medium/small, style=default/purple)
2. AvatarRectangle (variants=residential/commercial, fill=placeholder/image, size=medium/small, src=string/undefined, alt=string)

and to complete 2 of them, I've planned to have 3 pr:

1. Avatar (circle/square) (refactor)
2. AvatarRectangle (residential) (new)
3. AvatarRectangle (commercial) (new)

code snippets

// avatar
<span role="presentation" {... props}> // role can be overriden later
src (as <Icon/> or string)
</span>

// avatar-rectangle
<span role="presentation" classname="el-avatar-rectangle-container" {... props}> // role can be overriden later
(image or placeholder image) as <img classname="el-avatar-rectangle-residential" />
//or
(image or placeholder image) as <img classname="el-avatar-rectangle-commercial" />
<img classname="el-avatar-rectangle-commercial-layer" />
</span>

Should there be any incorrect component or prop names or any other, I would be happy to discuss it.

cc: @kurtdoherty @adrian-reapit

[kurtdoherty]

@nurm717123 I don't think AvatarRectangle is required at this stage (I've asked for confirmation from Andrei in the Figma file though).

question: I don't understand why you're opting to handle the icon via a src prop. Can you please explain your thinking here?

issue: I don't think we should use a prop called style for the default/purple option. I think we should treat that as a reserved prop given it's normally how inline styles would be supplied. We need to come up with a different name here; perhaps Design have some ideas. I've started a thread on the Figma file about this.

suggestion: I don't think we need an explicit variant: 'letters' | 'icon' prop. Seems more appropriate for us to handle that via children so that consumers do:

<Avatar shape="square">KD</Avatar>
<Avatar shape="circle">
  <MyIcon />
</Avatar>

Depending on how Andrei answers my questions on the Figma file, we may be able to use variant in place of shape.

[nurm717123]

@nurm717123 I don't think AvatarRectangle is required at this stage (I've asked for confirmation from Andrei in the Figma file though).

yes, I've seen your comment there, just updated the proposal couple hours ago but I think having 3 of them would be nice (in case table component would need this) so I revert it back cc @ksolanki7

question: I don't understand why you're opting to handle the icon via a src prop. Can you please explain your thinking here?

sure, i think its easier so that the consumer just need to use the variant prop to control what they want

issue: I don't think we should use a prop called style for the default/purple option

yes, I think the better name would be intent but I just follow what in the spec for initial

suggestion: I don't think we need an explicit variant: 'letters' | 'icon' prop. Seems more appropriate for us to handle that via children

ok, I got your point, I think better use this approach to make it the same pattern for new component 👍

[nurm717123]

and also there is another component e.g old Table component that use the old avatar, I think I will rename the previous avatar to DeperecatedAvatar , because the square and circle avatar didn't support image, yet.

[kurtdoherty]

suggestion: Deprecating sounds reasonable, but it could be good for you to communicate here what differences exist between the prop interfaces of the old and new avatar components. It may be simple to have some deprecated props on the new avatar that support what the existing one needs.

note: If you do take the deprecation route, please do all that work in a separate PR to the implementation of the new avatar component/s.

[nurm717123]

suggestion: Deprecating sounds reasonable, but it could be good for you to communicate here what differences exist between the prop interfaces of the old and new avatar components. It may be simple to have some deprecated props on the new avatar that support what the existing one needs.

Yes, basically the old avatar consumer is Nav (old), Card, Table (old), and PageHeader, and the main difference is:

1. it use type (profile/image) while the new version might use shape (circle/square)
2. it could use the children or it could use src props instead
3. The styling differs; for example, the old avatar with type=image (square) does not have a border radius.

note: If you do take the deprecation route, please do all that work in a separate PR to the implementation of the new avatar component/s.

Yes, sure 👍. I am leaning more toward it, but I still need input from others to decide whether we want to require existing consumers to adopt the newest design or not 🤔.

[nurm717123]

Great 👍, then based on the last comment I am planning to create 3 pr for this:

1. deprecate previous avatar
2. Avatar (circle/square)
3. AvatarRectangle

the reason I do the deprecation first is because the previous avatar is supporting src which in the new version does not (need to use AvatarRectangle instead). please let me know if I need to separate more, thanks.

[kurtdoherty]

Yep, looks good @nurm717123 👍

suggestion: When it comes to AvatarRectangle, it would be worth trying out the <object> element. It supports behaviour like:

<object
  data="path/to/main/image.png"
  type="image/png"
  width="600"
  height="140">
  <img src="path/to/fallback/image.jpg" />
</object>

Where the <img> is used only as a fallback if the object element's data fails to load. It may even be possible for us to use an <svg> element or data URL.

[nurm717123]

Absolutely, I’d like to give that a try first. Thank you

[nurm717123]

Hi @kurtdoherty after I tried the object, I found some issue to implement it to AvatarRectangle which is:

1. it requires to specify type (e.g type="image/jpeg") to display the image (especially jpeg) correctly
   e.g and the scrollbar couldn't be hidden from css
2. it also doesn't support image-fit props like have (fill/contain/cover .etc in case consumer want to custom it)

https://codesandbox.io/p/sandbox/3w9jt5

I think img tag is enough for AvatarRectangle and easy to use.

[kurtdoherty]

question: You seem to suggest that specifying the mime type is not acceptable? If so, why?

note: By not specifying the mime type, I believe the browser defaults to displaying the object via an iframe, which is why you see scrollbars and need to target a child <html> element.

note: I believe you mean object-fit, and it seems to work fine...

Here's an updated sandbox: https://codesandbox.io/p/sandbox/3w9jt5

[nurm717123]

Here's an updated sandbox: https://codesandbox.io/p/sandbox/3w9jt5

the sandbox seems not updated 👀

question: You seem to suggest that specifying the mime type is not acceptable? If so, why?

yes, I think that it require us to specify the valid image type (e.g only jpg image is allowed to use this component)

By not specifying the mime type, I believe the browser defaults to displaying the object via an iframe, which is why you see scrollbars and need to target a child element.

I have tried it in the sandbox by adding overflow hidden to the html of the object but still doesn't work (it work if I did it in devtool though 🤔 ).

[nurm717123]

I have tried it in the sandbox by adding overflow hidden to the html of the object but still doesn't work (it work if I did it in devtool though 🤔 ).

turns out it a browser specific issue, I'm using Brave browser by default (I've tried in Chrome too) it doesn't work, but in Firefox it works

[kurtdoherty]

🤦 Sorry! I didn't use the right link... here's the updated one: https://codesandbox.io/p/sandbox/tender-chaplygin-gkclqq

question: You seem to suggest that specifying the mime type is not acceptable? If so, why?
yes, I think that it require us to specify the valid image type (e.g only jpg image is allowed to use this component)

We can use any valid mime type, so long as it matches to the kind of resource we're referencing in the data attribute. I would expect the mime type to be known by consumers, so it feels fine to require them to explicitly provide it 🤷‍♂️

That said, we don't have to use <object>. I just liked the built-in fallback behaviour it provided. It looks like we may lose some performance related options that maybe aren't things we want to lose, like the loading attribute that <img> has... I can't find any good docs on how <object> behaves in this regard, so I don't know if it elements are always lazily loaded, or always eagerly loaded.

It's fine if you want to take the route and leverage onerror for detecting when we need to show the fallback.

[nurm717123]

ok, I will use onError for that, and for html user I think a classname that is actually box colored grey (just like the design spec) would do (e.g commercial-placeholder something like that)👍