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.

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

Add Image resizeMode center to iOS #8792

Closed
wants to merge 2 commits into from
Closed

Add Image resizeMode center to iOS #8792

wants to merge 2 commits into from

Conversation

aleclarson
Copy link
Contributor

@aleclarson aleclarson commented Jul 14, 2016 •
edited
Loading

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

ghost commented Jul 14, 2016

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

@ghost ghost 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
javache
javache reviewed Jul 15, 2016
View reviewed changes
Libraries/Image/RCTImageUtils.m

// Make sure the image is not clipped by the target.
if (sourceSize.height > destSize.height) {
sourceSize.width = destSize.width = destSize.width;
Copy link
Member

Choose a reason for hiding this comment

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

This expression seems incorrect

Copy link
Contributor Author

@aleclarson aleclarson Jul 15, 2016 •
edited
Loading

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
Member

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

@ghost ghost 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
@ghost ghost 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
Contributor Author

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

@ghost ghost 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
Member

javache commented Jul 19, 2016

@facebook-github-bot shipit

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

ghost commented Jul 19, 2016

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

@ghost ghost closed this in bee1180 Jul 19, 2016
fadils pushed a commit to fadils/react-native that referenced this pull request Jul 21, 2016
@aleclarson @fadils
Add Image resizeMode center to iOS
4e5d583 
Summary:
Addresses this comment: facebook#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)
Closes facebook#8792

Differential Revision: D3586134

Pulled By: javache

fbshipit-source-id: 78fb8e5928284003437dac2c9ad264fa584f73ec
samerce pushed a commit to iodine/react-native that referenced this pull request Aug 23, 2016
@aleclarson @samerce
Add Image resizeMode center to iOS
4cdcd11 
Summary:
Addresses this comment: facebook#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)
Closes facebook#8792

Differential Revision: D3586134

Pulled By: javache

fbshipit-source-id: 78fb8e5928284003437dac2c9ad264fa584f73ec
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
@aleclarson
Add Image resizeMode center to iOS
23f224a 
Summary:
Addresses this comment: facebook#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)
Closes facebook#8792

Differential Revision: D3586134

Pulled By: javache

fbshipit-source-id: 78fb8e5928284003437dac2c9ad264fa584f73ec
@aleclarson aleclarson deleted the resizemode-center-ios branch September 8, 2016 15:47
@motiz88 motiz88 mentioned this pull request Jan 27, 2018
Closed
facebook-github-bot pushed a commit that referenced this pull request Jan 27, 2018
@motiz88 @facebook-github-bot
Test and document Image resizeMode=center on iOS
be7037f 
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
@aleclarson @seraj-ahmad
Add Image resizeMode center to iOS
354e33c 
Summary:
Addresses this comment: facebook/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)
Closes facebook/react-native#8792

Differential Revision: D3586134

Pulled By: javache

fbshipit-source-id: 78fb8e5928284003437dac2c9ad264fa584f73ec
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aleclarson @javache