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(image): support for resizeMode and objectFit value of 'none' #47110

Conversation

mateoguzmana
Copy link
Contributor

@mateoguzmana mateoguzmana commented Oct 18, 2024

Summary:

As part of #34425, objectFit value of 'none' needs to be supported for the Image component.

In order to support this, a new value must also be added to support the equivalent in resizeMode. With this new value, the image will not be resized at all and keeping it in the initial position within a container (see in the screenshots).

In this PR the support is added for both Fabric and Paper.

Changelog:

[GENERAL] [ADDED] - image resizeMode and objectFit support for 'none'.

Test Plan:

Using the rn-tester, there is a new image example for both resizeMode and objectFit.

See below the results for both Android and iOS:

Fabric screenshots

Android:

Resize Mode Object Fit
Screenshot_1729232899 Screenshot_1729232912

iOS:

Resize Mode Object Fit
Simulator Screenshot - iPhone 16 Pro Max - 2024-10-18 at 08 16 37 Simulator Screenshot - iPhone 16 Pro Max - 2024-10-18 at 08 16 55
Paper screenshots

Android:

Resize Mode Object Fit
Screenshot_1729286528 Screenshot_1729286542

iOS:

Resize Mode Object Fit
Simulator Screenshot - iPhone 16 Pro Max - 2024-10-18 at 22 21 22 Simulator Screenshot - iPhone 16 Pro Max - 2024-10-18 at 22 21 16

@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 Oct 18, 2024
@mateoguzmana mateoguzmana marked this pull request as ready for review October 18, 2024 06:39
@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 Oct 18, 2024
@javache
Copy link
Member

javache commented Oct 18, 2024

Thanks for the PR! The native changes seem relatively simple so happy to merge this. Please have a look if you can make this work with Paper too, so we don't need to add a compatibility notice (I think it will already work on Android paper as-is). If it adds additional complexity, let's leave it as-is.

@react-native-bot
Copy link
Collaborator

Fails
🚫

Danger failed to run dangerfile.js.

Error ReferenceError

validateChangelog is not defined
ReferenceError: validateChangelog is not defined
    at Object.<anonymous> (dangerfile.js:48:18)
    at Module._compile (node:internal/modules/cjs/loader:1364:14)
    at requireFromString (/home/runner/work/react-native/react-native/node_modules/require-from-string/index.js:28:4)
    at /home/runner/work/react-native/react-native/node_modules/danger/distribution/runner/runners/inline.js:161:68
    at step (/home/runner/work/react-native/react-native/node_modules/danger/distribution/runner/runners/inline.js:52:23)
    at Object.next (/home/runner/work/react-native/react-native/node_modules/danger/distribution/runner/runners/inline.js:33:53)
    at /home/runner/work/react-native/react-native/node_modules/danger/distribution/runner/runners/inline.js:27:71
    at new Promise (<anonymous>)
    at __awaiter (/home/runner/work/react-native/react-native/node_modules/danger/distribution/runner/runners/inline.js:23:12)
    at runDangerfileEnvironment (/home/runner/work/react-native/react-native/node_modules/danger/distribution/runner/runners/inline.js:118:132)
    at /home/runner/work/react-native/react-native/node_modules/danger/distribution/platforms/GitHub.js:181:38
    at step (/home/runner/work/react-native/react-native/node_modules/danger/distribution/platforms/GitHub.js:44:23)
    at Object.next (/home/runner/work/react-native/react-native/node_modules/danger/distribution/platforms/GitHub.js:25:53)
    at /home/runner/work/react-native/react-native/node_modules/danger/distribution/platforms/GitHub.js:19:71
    at new Promise (<anonymous>)
    at __awaiter (/home/runner/work/react-native/react-native/node_modules/danger/distribution/platforms/GitHub.js:15:12)
    at Object.executeRuntimeEnvironment (/home/runner/work/react-native/react-native/node_modules/danger/distribution/platforms/GitHub.js:144:88)
    at /home/runner/work/react-native/react-native/node_modules/danger/distribution/commands/danger-runner.js:101:47
    at step (/home/runner/work/react-native/react-native/node_modules/danger/distribution/commands/danger-runner.js:34:23)
    at Object.next (/home/runner/work/react-native/react-native/node_modules/danger/distribution/commands/danger-runner.js:15:53)
    at fulfilled (/home/runner/work/react-native/react-native/node_modules/danger/distribution/commands/danger-runner.js:6:58)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Dangerfile

43|     'This will require a manual import by a Facebook employee.';
44|   warn(`${title} - <i>${idea}</i>`);
45| }
46| 
47| // Provides advice if a test plan is missing.
--------------------^
48| const includesTestPlan =
49|   danger.github.pr.body &&
50|   danger.github.pr.body.toLowerCase().includes('## test plan');
51| if (!includesTestPlan && !isFromPhabricator) {

Generated by 🚫 dangerJS against 4af97a8

@mateoguzmana
Copy link
Contributor Author

Thanks for taking a look @javache! I've added the support for Paper as well in 4af97a8

facebook-github-bot pushed a commit that referenced this pull request Oct 21, 2024
Summary:
While working on #47110, I wanted to disable fabric in the `rn-tester` to test some things in Paper. I followed all the steps but ended up deleting the whole repository locally, cloning it again and explicitly installing the pods `fabric_enabled` in all steps and it didn't work. I ended up disabling the new architecture by passing `RCT_NEW_ARCH_ENABLED=0` and then it worked immediately.

Wanted to add this extra hint as it might help other contributors.

## Changelog:

[INTERNAL] [ADDED] - extra instructions to disable fabric in the `rn-tester` package

Pull Request resolved: #47127

Test Plan: Follow the instructions to disable fabric, try passing `RCT_NEW_ARCH_ENABLED=0` if `fabric_enabled = false` is not enough.

Reviewed By: cortinico

Differential Revision: D64652149

Pulled By: rshest

fbshipit-source-id: 0fc4149832a7973c57161b4fff5815414f304a3c
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Nov 5, 2024
@facebook-github-bot
Copy link
Contributor

@javache merged this pull request in d8cfd98.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @mateoguzmana in d8cfd98

When will my fix make it into a release? | How to file a pick request?

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.

4 participants