react icon indicating copy to clipboard operation
react copied to clipboard

Feature/3606 enhance avatar image src

Open timges opened this issue 1 year ago • 4 comments

Closes #3606

Changelog

New

Changed

  • Avatar: Expanded the src prop 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)

timges avatar Feb 19 '24 16:02 timges

🦋 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

changeset-bot[bot] avatar Feb 19 '24 16:02 changeset-bot[bot]

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:

  1. Remove the DEFAULT_AVATAR_SIZE completely and calculate a default size value based on the width and height to maintain the squared/circular shape
  2. Keep it as is and apply the DEFAULT_AVATAR_SIZE, not matter what the type of the src is.

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. 😊

timges avatar Feb 26 '24 09:02 timges

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 🙏🏻

broccolinisoup avatar Mar 11 '24 02:03 broccolinisoup

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 😌👍

timges avatar Mar 11 '24 08:03 timges