Feature/3606 enhance avatar image src
Closes #3606
Changelog
New
Changed
-
Avatar: Expanded the
srcprop to allow not only strings but also image data objects as a source for images. These objects must contain a src key.
Removed
Rollout strategy
- [ ] Patch release
- [x] Minor release
- [ ] Major release; if selected, include a written rollout or migration plan
- [ ] None; if selected, include a brief description as to why
Testing & Reviewing
QUESTION: The created ImageData type is basicaly just a proxy for the StaticImageData type which next.js provides. I did this because I was not sure if installing the actual types of next js is a valid option, or not. What do you think?
Merge checklist
- [x] Added/updated tests
- [x] Added/updated documentation
- [x] Added/updated previews (Storybook)
- [x] Changes are SSR compatible
- [x] Tested in Chrome
- [x] Tested in Firefox
- [x] Tested in Safari
- [x] Tested in Edge
- [ ] (GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)
🦋 Changeset detected
Latest commit: fdcd84dd92fbe69249823d2e089edafe26f429c9
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 1 package
| Name | Type |
|---|---|
| @primer/react | Minor |
Not sure what this means? Click here to learn what changesets are.
Click here if you're a maintainer who wants to add another changeset to this PR
Hey @broccolinisoup ✌🏼. Thank you so much for the review! 😇
Your thoughts on integrating the image data's width and height make a lot of sense and feel quite natural. If this would be a plain Image component i would agree with you 100%!
However, as I thought about it some more, I figured that it might mess with the Avatar's consistency. Right now, the Avatar's default size is set at 20x20px, probably chosen to suit the most common uses (just guessing here). It ensures that the avatar image has equal width and height values, so that the user will always see a nice circular or squared shape rather than an elliptical or rectangular one, when the values differ.
By not applying the default size when the src is of the type image data, users might see different outcomes for the same image depending on the type of src they choose. It's not a huge deal since users can define the size anyway, but it just doesn't sit quite right with me. I would love to get your opinion on this! 🙂
I see two options, which might make sense here:
- Remove the
DEFAULT_AVATAR_SIZEcompletely and calculate a defaultsizevalue based on thewidthandheightto maintain the squared/circular shape - Keep it as is and apply the
DEFAULT_AVATAR_SIZE, not matter what the type of thesrcis.
Currently i feel like option 2 would be the best fit, as it is non breaking an keeps the component consistent in its default configuration. No matter what image is used. I might of course be missing better solutions!
Let me know if something was unclear. I'm excited to hear your thoughts. 😊
Hi @timges 👋🏻 Sorry for the late response.
I see your point about not automatically passing the width and height to the avatar component and it makes a lot of sense! I now have concerns about the need for passing an object though if we are not leveraging passing the width and height and all these additional props 🤔
I'd like to take this back to chat about the API with my team and get back to you with a little clearer feedback. Thanks for your understanding 🙏🏻
Hi @broccolinisoup ✌🏼,
Don't worry; the PR isn't going to escape us 😬
I see where you're coming from and agree that more discussion is necessary. Since this feature is mainly focused on improving convenience for a specific set of users, it doesn't seem pressing anyways. Should you require more input, perspectives, or assistance, feel free to reach out, and I'll be happy to chip in 😌👍