Skip to content

Add Image resizeMode center to iOS - #8792

Closed
aleclarson wants to merge 2 commits into
react:masterfrom
aleclarson:resizemode-center-ios
Closed

Add Image resizeMode center to iOS#8792
aleclarson wants to merge 2 commits into
react:masterfrom
aleclarson:resizemode-center-ios

Conversation

@aleclarson

@aleclarsonaleclarson commented Jul 14, 2016

Copy link
Copy Markdown
Contributor

Addresses this comment: #2296 (comment)

This pull request adds the center value to ImageResizeMode.
When set, it will center the image within its frame.
If the image is larger than its frame, the image is downscaled while maintaining its aspect ratio.
That is how the Android implementation works, too.

Sorry, don't have time to write tests. 😢

Any reviewers should make sure RCTTargetRect returns the correct value when:

  • the image is smaller than its frame (ie: no downscaling needed)
  • the image is larger than its frame (should be downscaled to avoid clipping)

@ghost

Copy link
Copy Markdown

By analyzing the blame information on this pull request, we identified @JoelMarcey and @nicklockwood to be potential reviewers.

@ghostghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 14, 2016
Comment threadLibraries/Image/RCTImageUtils.m Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This expression seems incorrect

@aleclarsonaleclarsonJul 15, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@javache
Are you talking about destSize.width = destSize.width (which is used elsewhere so I just copied it)?
Or do you mean I should be using the aspect ratios instead?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could be a bug in the other code then too. Could you clean up all of them up? It probably should just be sourceSize.width = destSize.width

@ghostghost 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 Jul 15, 2016
@aleclarson
aleclarsonforce-pushed the resizemode-center-ios branch from 039b293 to 7b591f4CompareJuly 18, 2016 22:44
@ghostghost 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 Jul 18, 2016
@aleclarson

Copy link
Copy Markdown
ContributorAuthor

@javache Added another commit that removes the extra assignments. 👍

@ghostghost 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 Jul 18, 2016
@javache

Copy link
Copy Markdown
Contributor

@facebook-github-bot shipit

@ghostghost added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. and removed GH Review: review-needed labels Jul 19, 2016
@ghost

Copy link
Copy Markdown

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

@ghostghost closed this in bee1180Jul 19, 2016
@aleclarson
aleclarson deleted the resizemode-center-ios branch September 8, 2016 15:47
facebook-github-bot pushed a commit that referenced this pull request Jan 27, 2018
Summary:
`<Image resizeMode="center">` already works on iOS (implemented in #8792), but is neither tested nor documented the way the other `resizeMode` values are.
This PR primarily enables the relevant RNTester case on iOS, and secondarily copies over the doc comment from `Image.android.js` to `Image.ios.js`. A PR to `react-native-website` will follow shortly and it is there I will try and revise the wording a bit.
Updated RNTester screenshot (iOS):
<img src=https://user-images.githubusercontent.com/2246565/35470720-44b38282-0357-11e8-941c-1b3c5a1b2f3b.png width=300>
react-native-website PR coming soon.
[IOS] [MINOR] [Image] - Include resizeMode=center in RNTester
Closes#17759
Differential Revision: D6829051
Pulled By: hramos
fbshipit-source-id: c6e0000a75765e8bf3a1d0306aaafad002b14a58
tungdo194 pushed a commit to tungdo194/rn-test that referenced this pull request Apr 28, 2024
Summary:
Addresses this comment: react/react-native#2296 (comment)
This pull request adds the `center` value to `ImageResizeMode`.
When set, it will center the image within its frame.
If the image is larger than its frame, the image is downscaled while maintaining its aspect ratio.
That is how the Android implementation works, too.
Sorry, don't have time to write tests. 😢
Any reviewers should make sure `RCTTargetRect` returns the correct value when:
- the image is smaller than its frame (ie: no downscaling needed)
- the image is larger than its frame (should be downscaled to avoid clipping)
Closesreact/react-native#8792
Differential Revision: D3586134
Pulled By: javache
fbshipit-source-id: 78fb8e5928284003437dac2c9ad264fa584f73ec
This pull request was closed.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.Import StartedThis pull request has been imported. This does not imply the PR has been approved.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@aleclarson@javache