[iOS] Fabric: Support defaultSource prop of Image component#46554
[iOS] Fabric: Support defaultSource prop of Image component#46554zhongwuzw wants to merge 3 commits into
Conversation
facebook-github-bot
added
CLA Signed
cipolleschi
left a comment
cipolleschi
left a comment
There was a problem hiding this comment.
Some small nits to update.
I'm not super familiar with the Image logic, once we fix the nits, I'll import it and ask for few more people to have a look at it.
| if (strongSelf) { | ||
| if (!strongSelf->_imageView.image) { |
There was a problem hiding this comment.
| if (strongSelf) { | |
| if (!strongSelf->_imageView.image) { | |
| if (strongSelf && !strongSelf->_imageView.image) { |
but what if we have an image already? I mean, what happens if we change the imageSource prop?
There was a problem hiding this comment.
If we have an image already, we would not reset default image and just only to load new image source. The same logic as old arch.
| RCTImageComponentView *strongSelf = weakSelf; | ||
| if (strongSelf) { | ||
| if (!strongSelf->_imageView.image) { | ||
| strongSelf->_imageView.image = finalImage; |
There was a problem hiding this comment.
Shouldn't we emit an OnLoadEnd event here?
There was a problem hiding this comment.
The same as old arch, we only fire image load events for image source.
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…/fabric_image_default_source # Conflicts: # packages/react-native/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm update
|
@cipolleschi Hi, just a friendly ping, any review feedback about this PR? :) |
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
react-native-bot
added
Stale
|
update |
|
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
react-native-bot
added
Stale
4 participants
Summary:
We missed
defaultSourcesupport of Image in Fabric, so let's add it.Changelog:
[IOS] [FIXED] - Fabric: Support defaultSource prop of Image component
Test Plan:
RNTester Image
defaultSourceexample worked in Fabric.