Skip to content
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

feat: added crossOrigin, referrerPolicy, srcSet, width, height and src props to the Image Component. #34481

Closed

Conversation

dhruvtailor7
Copy link
Contributor

@dhruvtailor7 dhruvtailor7 commented Aug 23, 2022

Summary

This PR is for adding the support for crossOrigin, referrerPolicy, width, height and srcSet props to Image Component and mapping the src prop to source.uri of Image Component for the issue #34424. An example is also added in the RNTester ImageExample.

Changelog

[General] [Changed] - Map the src prop to source.uri prop in Image Component.
[General] [Added] - added crossOrigin, referrerPolicy, width, height and srcSet props to Image Component.

Test Plan

  1. Navigate to Image Component Example in the RNTester app.
  2. Contains an example of the Image component using the src and srcSet prop.
  3. For headers, inspect the Image request using Flipper.

@facebook-github-bot
Copy link
Contributor

Hi @dhruvtailor7!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@dhruvtailor7 dhruvtailor7 marked this pull request as draft August 23, 2022 12:35
@analysis-bot
Copy link

analysis-bot commented Aug 23, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,637,870 +835
android hermes armeabi-v7a 7,050,076 +842
android hermes x86 7,939,664 +826
android hermes x86_64 7,911,593 +822
android jsc arm64-v8a 9,513,421 +592
android jsc armeabi-v7a 8,288,943 +609
android jsc x86 9,452,810 +595
android jsc x86_64 10,043,953 +590

Base commit: 4f6929b
Branch: main

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 23, 2022
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@dhruvtailor7 dhruvtailor7 marked this pull request as ready for review August 24, 2022 06:14
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Aug 24, 2022
@analysis-bot
Copy link

analysis-bot commented Aug 24, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 4f6929b
Branch: main

@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.

@necolas
Copy link
Contributor

necolas commented Aug 24, 2022

Can we tackle all the new Image props at the same time, given that they all combine into the existing source prop?

e.g.,

const { crossOrigin, height, referrerPolicy, src, srcSet, width } = props;
let source = null;
if (src != null) {
  const headers = {};
  if (crossOrigin === 'use-credentials') {
    headers['Access-Control-Allow-Credentials'] = true;
  }
  if (referrerPolicy != null) {
    headers['Referrer-Policy'] = referrerPolicy;
  }
  nextProps.progressiveRenderingEnabled = true;
  source = { headers, height, uri: src, width };
}
if (srcSet != null) {
  source = [];
  const srcList = srcSet.split(', ');
  srcList.forEach((src) => {
    const [uri, xscale] = src.split(' ');
    const scale = parseInt(xscale.split('x')[0], 10);
    const headers = {};
    if (crossOrigin === 'use-credentials') {
      headers['Access-Control-Allow-Credentials'] = true;
    }
    if (referrerPolicy != null) {
      headers['Referrer-Policy'] = referrerPolicy;
    }
    source.push({ headers, height, scale, uri, width });
  });
}

@dhruvtailor7 dhruvtailor7 changed the title feat: map src prop to source.uri in Image Component feat: added crossOrigin, referrerPolicy, srcSet and src props to the Image Component. Aug 26, 2022
Libraries/Image/Image.android.js Outdated Show resolved Hide resolved
Libraries/Image/Image.android.js Outdated Show resolved Hide resolved
@dhruvtailor7
Copy link
Contributor Author

@necolas on Android, if we have multiple items in the source prop, and we do not provide width/height on each item, than an error is thrown from the native side. So, if we map multiple sources using the srcSet prop then the same error will be thrown as there is no way to provide width/height to each item using srcSet.

@necolas
Copy link
Contributor

necolas commented Aug 27, 2022

The same width and height should be passed for each source

@necolas
Copy link
Contributor

necolas commented Aug 31, 2022

@dhruvtailor7 are you planning to update this PR? Thanks

@dhruvtailor7
Copy link
Contributor Author

dhruvtailor7 commented Sep 1, 2022

@dhruvtailor7 are you planning to update this PR? Thanks

@necolas Yes, I am still looking into the failing flow test cases.

@dhruvtailor7 dhruvtailor7 changed the title feat: added crossOrigin, referrerPolicy, srcSet and src props to the Image Component. feat: added crossOrigin, referrerPolicy, srcSet, width, height and src props to the Image Component. Sep 2, 2022
@dhruvtailor7 dhruvtailor7 requested review from necolas and ecreeth and removed request for necolas and ecreeth September 5, 2022 05:48
@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.

Libraries/Image/ImageProps.js Outdated Show resolved Hide resolved
Copy link
Contributor

@necolas necolas left a comment

Choose a reason for hiding this comment

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

Good to see unit tests!

Libraries/Image/ImageSourceUtils.js Outdated Show resolved Hide resolved
Libraries/Image/ImageSourceUtils.js Outdated Show resolved Hide resolved
Libraries/Image/ImageSourceUtils.js Outdated Show resolved Hide resolved
Libraries/Image/ImageSourceUtils.js Outdated Show resolved Hide resolved
Libraries/Image/ImageSourceUtils.js Outdated Show resolved Hide resolved
Libraries/Image/ImageSourceUtils.js Outdated Show resolved Hide resolved
@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.

@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 pull request was successfully merged by @dhruvtailor7 in 47a05bc.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Sep 7, 2022
@dhruvtailor7 dhruvtailor7 deleted the feat/image-map-src-to-source branch September 23, 2022 11:00
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…ht` and `src` props to the Image Component. (facebook#34481)

Summary:
This PR is for adding the support for `crossOrigin`, `referrerPolicy`, `width`, `height` and `srcSet` props to Image Component and mapping the `src` prop to `source.uri` of Image Component for the issue facebook#34424. An example is also added in the RNTester ImageExample.

## Changelog

[General] [Changed] - Map the `src` prop to `source.uri` prop in Image Component.
[General] [Added] - added `crossOrigin`, `referrerPolicy`, `width`, `height` and `srcSet` props to Image Component.

Pull Request resolved: facebook#34481

Test Plan:
1. Navigate to Image Component Example in the RNTester app.
2. Contains an example of the Image component using the `src` and `srcSet` prop.
3. For headers, inspect the Image request using Flipper.

<img src="https://user-images.githubusercontent.com/32268377/186153246-d7b72ce3-e082-46d9-87d1-aefacd3af34f.png" height="500" />

Reviewed By: christophpurrer

Differential Revision: D38982041

Pulled By: cipolleschi

fbshipit-source-id: dd6594e39b8f3b36cfcdafa35695254034f1fb7f
facebook-github-bot pushed a commit that referenced this pull request Oct 18, 2024
Summary:
Fixes this: #41151

## Changelog:

[GENERAL] [FIXED] - Passed height and width as native props to support cases where source is an array.

**Issue**: Unable to use height and width as props in Image components when passing source as an array.

This functionality worked before this [commit](#34481). In Image.android.js and Image.ios.js, height and width props are extracted before passing them to the native side. This extraction makes these props ineffective when passed directly. Although setting height and width in style is a workaround, these props should still be respected since they exist for this purpose.

The issue occurs only when the source prop is an array. When source is an object, width and height are manually passed to the style object, so the issue doesn't arise.

Pull Request resolved: #47044

Test Plan: Tested this on `rn-tester` app

Reviewed By: necolas, javache

Differential Revision: D64458292

Pulled By: cipolleschi

fbshipit-source-id: 5cacad79b8d4468a4a1fd0977221e42ce4b2e5d1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants