Skip to content

[iOS] Fabric: Support defaultSource prop of Image component #46554

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
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[iOS] Fabric: Support defaultSource prop of Image component #46554

wants to merge 3 commits into from
+114 −27

Conversation

@zhongwuzw
Copy link
Contributor

Summary:

We missed defaultSource support of Image in Fabric, so let's add it.

Changelog:

[IOS] [FIXED] - Fabric: Support defaultSource prop of Image component

Test Plan:

RNTester Image defaultSource example worked in Fabric.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Sep 18, 2024
cipolleschi
cipolleschi requested changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

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.

packages/react-native/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm Outdated
Comment on lines 133 to 134
if (strongSelf) {
if (!strongSelf->_imageView.image) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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?

Copy link
Contributor Author

@zhongwuzw zhongwuzw Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

packages/react-native/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm Outdated
RCTImageComponentView *strongSelf = weakSelf;
if (strongSelf) {
if (!strongSelf->_imageView.image) {
strongSelf->_imageView.image = finalImage;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we emit an OnLoadEnd event here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same as old arch, we only fire image load events for image source.

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@zhongwuzw
Merge commit '5ed486cc8fb4aeef12c92a04619cc668427eee75' into features…
be0a8dd 
…/fabric_image_default_source

# Conflicts:
#	packages/react-native/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm
update
@zhongwuzw
Copy link
Contributor Author

@cipolleschi Hi, just a friendly ping, any review feedback about this PR? :)

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

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 react-native-bot added Stale There has been a lack of activity on this issue and it may be closed soon. and removed Stale There has been a lack of activity on this issue and it may be closed soon. labels Oct 22, 2025
@zhongwuzw
Copy link
Contributor Author

update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cipolleschi cipolleschi cipolleschi approved these changes

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zhongwuzw @facebook-github-bot @react-native-bot @cipolleschi