Implement Avatar component #2104
Conversation
solimant
requested review from
dannify,
devongovett and
snowystinger
as code owners
| /** | ||
| * The image URL for the avatar. | ||
| */ | ||
| src: string |
There was a problem hiding this comment.
technically this is optional https://spectrum.adobe.com/page/avatar/#-Use-generic-avatars-when-image-is-undefined
There was a problem hiding this comment.
My understanding of this guideline is that the client should use a generic avatar if their user/organization has not specified one.
There was a problem hiding this comment.
Ah yep, we interpreted it two different ways. Let's get some more opinions.
Down sides of both:
- optional image src means we would need to embed some images by default or code it to a discoverable place that we control so it can't be changed without us knowing
- required img src means potential for people forgetting to implement the generic icon or getting it from the wrong place
| /** | ||
| * The image URL for the avatar. | ||
| */ | ||
| src: string |
There was a problem hiding this comment.
My understanding of this guideline is that the client should use a generic avatar if their user/organization has not specified one.
solimant
force-pushed
the
1345
branch
2 times, most recently
from
d78cc13 to
d51a424
Compare
There was a problem hiding this comment.
@snowystinger, @LFDanLu - thank you for the review. I've addressed your comments/requests. Happy to address any further asks.
solimant
force-pushed
the
1345
branch
4 times, most recently
from
791a213 to
50e15a6
Compare
There was a problem hiding this comment.
I got clarification from design, this is how the 'size' prop should behave
<Avatar size="avatar-size-100" /> // allowed
<Avatar size="600px" /> // allowed
<Avatar size="30em" /> // allowed
<Avatar size="100" /> // NOT allowed
<Avatar size="size-100" /> // NOT allowed
<Avatar height="anything" /> // NOT allowed at all
<Avatar width="anything" /> // NOT allowed at all
and <Avatar size={100} /> behaves the same as <Avatar size="100px" />
solimant
force-pushed
the
1345
branch
4 times, most recently
from
fc6c424 to
da44ae4
Compare
There was a problem hiding this comment.
@LFDanLu - I've made the changes, please have another look.
i did just notice that this is a size prop, not width, so this may be less of an issue
though maybe we need to remove width/height as props that Avatar accepts?
@snowystinger - yes, we're removing width and height in SpectrumAvatarProps.
I got clarification from design, this is how the 'size' prop should behave
@snowystinger - thank you for checking. I've approached in a way that should satisfy that ask. I'm making an assumption, though, that disallowed values default to the default value; i.e. avatar-size-100. Please have another look.
There was a problem hiding this comment.
@snowystinger - I've addressed your comments. Please have a look. Thank you 🙏🏼
| * component. An avatar is a thumbnail representation of an entity, such as a | ||
| * user or an organization. | ||
| */ | ||
| export function useAvatar(props: AriaAvatarOptions): AvatarAria { |
There was a problem hiding this comment.
we can leave this here for now because the packages are still private and we may decide to move useAvatar into the Avatar component as it may be a very specific to Spectrum component
| Disabled.storyName = 'isDisabled'; | ||
|
|
||
| export const CustomSize = AvatarTemplate.bind({}); | ||
| CustomSize.args = {size: 'size-1000', src: SRC_URL_2}; |
There was a problem hiding this comment.
This one is an invalid size now, it'll just do default
| CustomSize.args = {size: 'size-1000', src: SRC_URL_2}; | |
| CustomSize.args = {size: 100, src: SRC_URL_2}; |
|
@snowystinger, @LFDanLu - thank you very much for the review and comments. I've addressed the requested changes, and have also introduced support for more element types via 16f710c. I'm happy to omit those changes and refactor whatever is necessary. Also, on a similar note, and further to our offline conversations, I understand we want to repurpose |
|
cc @dannify regarding the above. We talked about this offline previously, do we want to go ahead and move the hook's contents into the Avatar component for now? IMO |
|
I think for the first release I would like to merge the hook into React Spectrum. I think we will need it down the track but we don't have enough of a use case to make a pattern out of it just yet. It will likely be used as some sort of focusable image hook but we probably need to figure out what else will use it before we can generalize it and I'd rather not release it now because then we are stuck with the package. |
|
Thank you, @dannify - consolidation is done and ready for review. |
Closes #1345
✅ Pull Request Checklist:
📝 Test Instructions:
AvatarstorybookAvatarcan receive focus and show a focus ring🧢 Your Project:
Adobe/RSP