Skip to content

add imageProps prop to Avatar #7570

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

Jump to bottom
Merged
merged 2 commits into from
Apr 10, 2025
Merged

add imageProps prop to Avatar #7570

merged 2 commits into from
Apr 10, 2025
+7 −1

Conversation

max-capozzi-hpe
Copy link
Contributor

@max-capozzi-hpe max-capozzi-hpe commented Apr 2, 2025 •
edited
Loading

What does this PR do?

adds the imageProps prop to Avatar to add missing props for the Image used internally by Avatar

Where should the reviewer start?

Avatar.js

What testing has been done on this PR?

manually tested with Storybook

How should this be manually tested?

Storybook

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

What are the relevant issues?

#6530

Screenshots (if appropriate)

Screen Recording 2025-04-02 at 5 06 14 PM

Do the grommet docs need to be updated?

Yes

Should this PR be mentioned in the release notes?

Yes

Is this change backwards compatible or is it a breaking change?

Backwards compatible

taysea
taysea reviewed Apr 2, 2025
View reviewed changes
Copy link
Collaborator

@taysea taysea left a comment

Choose a reason for hiding this comment

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

This seems like a reasonable enhancement, thank you!

@jcfilben Are you comfortable with this API? Would there be any reason instead to structure as imageProps similar to dropProps on other components?

So instead of

<Avatar fallback="" />

it would be

<Avatar imageProps={{ fallback: '' }} />

Not sure if any of the other image props would ever be relevant in Avatar scenario.

@taysea taysea requested a review from jcfilben April 2, 2025 21:44
@jcfilben
Copy link
Collaborator

jcfilben commented Apr 2, 2025

This seems like a reasonable enhancement, thank you!

@jcfilben Are you comfortable with this API? Would there be any reason instead to structure as imageProps similar to dropProps on other components?

So instead of
<Avatar fallback="" />
it would be
<Avatar imageProps={{ fallback: '' }} />
Not sure if any of the other image props would ever be relevant in Avatar scenario.

I like the idea of imageProps I could see people potentially wanting to pass an aria-label or opacity here in addition to fallback

@jcfilben jcfilben added the waiting Awaiting response to latest comments label Apr 3, 2025
@max-capozzi-hpe max-capozzi-hpe force-pushed the add-fallback-prop-to-avatar branch 2 times, most recently from 7749786 to 15241ad Compare April 4, 2025 19:12
@max-capozzi-hpe max-capozzi-hpe changed the title add fallback prop to Avatar add imageProps prop to Avatar Apr 4, 2025
@jcfilben jcfilben removed the waiting Awaiting response to latest comments label Apr 4, 2025
jcfilben
jcfilben approved these changes Apr 4, 2025
View reviewed changes
Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks @max-capozzi-hpe

@jcfilben jcfilben requested a review from taysea April 4, 2025 19:34
taysea
taysea approved these changes Apr 4, 2025
View reviewed changes
Copy link
Collaborator

@taysea taysea left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the contribution @max-capozzi-hpe!

@jcfilben
Copy link
Collaborator

jcfilben commented Apr 4, 2025

Waiting to merge until after we release v2.46.1

@max-capozzi-hpe
add imageProps to Avatar
0fa2187 
Signed-off-by: Max Capozzi <max.capozzi@hpe.com>
@max-capozzi-hpe max-capozzi-hpe force-pushed the add-fallback-prop-to-avatar branch from 15241ad to 0fa2187 Compare April 9, 2025 01:48
@jcfilben jcfilben merged commit 80ac437 into grommet:master Apr 10, 2025
13 of 14 checks passed
@jcfilben jcfilben mentioned this pull request Apr 22, 2025
Closed
This was referenced Apr 30, 2025
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taysea taysea taysea approved these changes

@jcfilben jcfilben jcfilben approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@max-capozzi-hpe @jcfilben @taysea