fix: multiple components render issue in Avatar #3831

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.

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

arpitBhalla merged 5 commits into react-native-elements:next from khushal87:khushal87/fix-avatar

Sep 9, 2023

Merged

fix: multiple components render issue in Avatar #3831

arpitBhalla merged 5 commits into react-native-elements:next from khushal87:khushal87/fix-avatar

Sep 9, 2023

Conversation

@khushal87


 fix: multiple components render issue in Avatar

d869fa3

@netlify

Copy link

netlify bot commented Aug 26, 2023 •

edited

Loading

✅ Deploy Preview for react-native-elements ready!

Name	Link
🔨 Latest commit	`8da88d0`
🔍 Latest deploy log	https://app.netlify.com/sites/react-native-elements/deploys/64f961e932f56300086a6a9d
😎 Deploy Preview	https://deploy-preview-3831--react-native-elements.netlify.app
📱 Preview on mobile	Toggle QR Code... QR Code Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@khushal87


 fix: multiple components render issue in Avatar

529a281

@khushal87 khushal87 requested a review from arpitBhalla

August 26, 2023 06:25

haeniya

haeniya reviewed

Aug 26, 2023

View reviewed changes

packages/base/src/Avatar/Avatar.tsx

let componentToRender;

if (source) {

Copy link

@haeniya haeniya Aug 26, 2023 •

edited

Loading

Choose a reason for hiding this comment

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

If I unserstand correctly, in 4.0.0-rc.7, the title (if present) was shown in two different situations:

As placeholder content when the image was rendering or could not be loaded
As a fallback, if there was no image source passed (e.g. when the image source is undefined)
See here: https://github.com/react-native-elements/react-native-elements/blob/v4.0.0-rc.7/packages/base/src/Avatar/Avatar.tsx#L206-L230

I wonder if this behavior is still the same with the latest changes.

There is a workaround for this (#3824 (comment)) but maybe we could keep the behavior the same? If this in on purpose and there are reasons to have it that way, it's fine for me. Just wanted to point that out.

Copy link

Member Author

@khushal87 khushal87 Aug 26, 2023

Choose a reason for hiding this comment

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

Hey @haeniya I totally understand your concern. I was checking the implementation of Placeholder content but regardless of this change, it doesn't seem to render the content even if I pass a placeholder content directly to the Image component(I doubt this ever worked before). The issue might be different and can be addressed differently.

This PR sought to fix the multiple components render issue(that was a mistake) and rendering placeholder still remains an issue.

Copy link

Member Author

@khushal87 khushal87 Aug 26, 2023

Choose a reason for hiding this comment

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

I think I would say using the imageProps would be a more viable option since we have it in Avatar. Users can use the imageProp and pass PlaceholerContent into it. No point in introducing a new prop in avatar for the same.

Copy link

@haeniya haeniya Aug 26, 2023

Choose a reason for hiding this comment

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

It worked in 4.0.0-rc.7 but doesn't in the 4.0.0-rc.8 release. But you're right, it could be handled in a different issue/PR. I was just wondering if we need to refactor the code anyway, why not fix that too (to maybe speed up the process a bit).

I think I would say using the imageProps would be a more viable option since we have it in Avatar. Users can use the imageProp and pass PlaceholerContent into it. No point in introducing a new prop in avatar for the same.

I think the idea would not be to introduce a new prop but to make the title prop the PlaceholderContent if present. The linked workaround works for me but I need to put the title twice, as PlaceholderContent and as title. Was quite convenient to have this fallback automatically.

Copy link

Member Author

@khushal87 khushal87 Aug 26, 2023

Choose a reason for hiding this comment

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

Hey @haeniya, I have pushed the icon as the placeholder component as per the docs. Hope that helps. Please review the PR, if you can and let me know if it works. 😄

Copy link

Member Author

@khushal87 khushal87 Sep 3, 2023

Choose a reason for hiding this comment

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

Hey @haeniya, I am sorry for the confusion. I was luckily able to reproduce it now and I have pushed the fix. Hope that helps. Thanks 😄

Copy link

Member Author

@khushal87 khushal87 Sep 6, 2023

Choose a reason for hiding this comment

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

@haeniya if you have tested it well, can we merge it?

Copy link

@haeniya haeniya Sep 6, 2023

Choose a reason for hiding this comment

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

@khushal87 I don't see any updates on the PR. Have you pushed your changes?

Copy link

Member Author

@khushal87 khushal87 Sep 7, 2023

Choose a reason for hiding this comment

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

Oh sorry my bad @haeniya, I thought I pushed it but I missed it somehow. Now I have done. Thanks 😄

Copy link

@haeniya haeniya Sep 7, 2023

Choose a reason for hiding this comment

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

@khushal87 I just tested it and it looks good to me. Thanks for the update. 👍

@khushal87


 fix: render icon as placeholder content

15161b6

@codecov

Copy link

codecov bot commented Aug 26, 2023 •

edited

Loading

Codecov Report

Merging #3831 (3b56302) into next (b5a95f6) will increase coverage by 0.03%.
Report is 1 commits behind head on next.
The diff coverage is 92.85%.

❗ Current head 3b56302 differs from pull request most recent head 8da88d0. Consider uploading reports for the commit 8da88d0 to get more accurate results

@@ Coverage Diff @@
## next #3831 +/- ##
==========================================
+ Coverage 79.87% 79.91% +0.03% 
==========================================
 Files 87 87 
 Lines 1824 1837 +13 
 Branches 814 809 -5 
==========================================
+ Hits 1457 1468 +11 
- Misses 362 364 +2 
 Partials 5 5

Files Changed	Coverage Δ
packages/base/src/Avatar/Avatar.tsx	`95.74% <92.85%> (-4.26%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

khushal87 added 2 commits

August 26, 2023 22:21

@khushal87


 fix: add title to placeholder content

1d22609

@khushal87


 fix: avatar placeholder styling

8da88d0

@arpitBhalla arpitBhalla merged commit 75d6ef7 into react-native-elements:next

Sep 9, 2023

@arpitBhalla arpitBhalla added the PR: Bug Fix 🐛 label

Sep 9, 2023

@l2aelba

Copy link

l2aelba commented Dec 14, 2023

How to install or update to this fix ? @khushal87

@l2aelba l2aelba mentioned this pull request

Dec 15, 2023

dependencies are not updated #3829

Closed

1 task

Labels

PR: Bug Fix 🐛

4 participants

@khushal87 @l2aelba @haeniya @arpitBhalla

Uh oh!

fix: multiple components render issue in Avatar #3831

fix: multiple components render issue in Avatar #3831

Uh oh!

Conversation

@khushal87 khushal87 commented Aug 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Type of change

How Has This Been Tested?

Checklist

Additional context

Uh oh!

netlify bot commented Aug 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for react-native-elements ready!

Uh oh!

@haeniya haeniya Aug 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@khushal87 khushal87 Aug 26, 2023

Choose a reason for hiding this comment

Uh oh!

@khushal87 khushal87 Aug 26, 2023

Choose a reason for hiding this comment

Uh oh!

@haeniya haeniya Aug 26, 2023

Choose a reason for hiding this comment

Uh oh!

@khushal87 khushal87 Aug 26, 2023

Choose a reason for hiding this comment

Uh oh!

@khushal87 khushal87 Sep 3, 2023

Choose a reason for hiding this comment

Uh oh!

@khushal87 khushal87 Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

@haeniya haeniya Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

@khushal87 khushal87 Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

@haeniya haeniya Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

l2aelba commented Dec 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

@khushal87 khushal87 commented Aug 26, 2023 •

edited

Loading

netlify bot commented Aug 26, 2023 •

edited

Loading

@haeniya haeniya Aug 26, 2023 •

edited

Loading

codecov bot commented Aug 26, 2023 •

edited

Loading