fix: multiple components render issue in Avatar #3831
Conversation
✅ Deploy Preview for react-native-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
|
||
| let componentToRender; | ||
|
|
||
| if (source) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
imagePropand 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.
There was a problem hiding this comment.
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. 😄
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
@haeniya if you have tested it well, can we merge it?
There was a problem hiding this comment.
@khushal87 I don't see any updates on the PR. Have you pushed your changes?
There was a problem hiding this comment.
Oh sorry my bad @haeniya, I thought I pushed it but I missed it somehow. Now I have done. Thanks 😄
There was a problem hiding this comment.
@khushal87 I just tested it and it looks good to me. Thanks for the update. 👍
Codecov Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
How to install or update to this fix ? @khushal87 |
Motivation
Fixes #3824
Fixes # (issue)
Type of change
How Has This Been Tested?
exampleappChecklist
yarn docs-build-apiAdditional context