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

Fix resizeMode="cover" when image is resized #1273

Merged
merged 2 commits into from
Sep 22, 2022
Merged

Fix resizeMode="cover" when image is resized #1273

merged 2 commits into from
Sep 22, 2022

Conversation

christophpurrer
Copy link

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Since RCTResizeModeCover is implemented manually on macOS (iOS benefits from UIViewContentModeScaleAspectFill API), we need to reprocess the image any time the bounds of the view update. The updateImage: method was already called on resize, so this instead stores the original image in an ivar so it can call RCTFillImagePreservingAspectRatio from updateImage: instead.

Changelog

[macOS] [Fixed] - Fix resizeMode="cover" when image is resized

Test Plan

Before

Look at the images labeled 'Cover' and observe how they don't properly resize

resizeBefore.mov

After

resizeAfter.mov

Before

Another demo, done internally

resizeBefore1.mov

After

resizeAfter2_.mov

@christophpurrer christophpurrer requested a review from a team as a code owner July 22, 2022 14:15
@pull-bot
Copy link

Messages
📖 📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.

Generated by 🚫 dangerJS against ff5ffb0

@christophpurrer
Copy link
Author

@mischreiber > Does this change look good to you?

@Saadnajmi Saadnajmi added the Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) label Aug 9, 2022
@Saadnajmi
Copy link
Collaborator

"Needs attention" label here means that we (Microsoft, aka @mischreiber) will take a deeper look. No changes requested from the PR author yet.

@christophpurrer
Copy link
Author

Anything needed from our side to get this change landed?

@Saadnajmi
Copy link
Collaborator

Anything needed from our side to get this change landed?

@mischrieber had wanted to take a deeper look, but is also currently OOF. Is this is blocking other changes from making their way through, we can prioritize this one over others. Come to think of it, it probably takes priority by default on account of being "older" than the others..

@appden @christophpurrer
Fix resizeMode="cover" when image is resized
0564c44 
Since `RCTResizeModeCover` is implemented manually on macOS (iOS benefits from `UIViewContentModeScaleAspectFill` API), we need to reprocess the image any time the bounds of the view update. The `updateImage:` method was already called on resize, so this instead stores the original image in an ivar so it can call `RCTFillImagePreservingAspectRatio` from `updateImage:` instead.
@appden @christophpurrer
Fix rendering template images with tintColor
d2b375f 
This fixes a regression introduced by the previous commit, where images would no longer render with the `tintColor` on release builds. We must call `setTemplate:` (using `.template` property is illegal in Objective-C++ since `template` is reserved in C++) on the newly resized image instead of the original image.

Confirmed icons look correct in the release build of Messenger Desktop.
Saadnajmi
Saadnajmi approved these changes Sep 22, 2022
View reviewed changes
Copy link
Collaborator

@Saadnajmi Saadnajmi left a comment

Choose a reason for hiding this comment

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

I'm going to go ahead and also approve this. It's an old enough , simple PR that I'd rather not linger. We can back it out if it causes issues, @mischreiber FYI

@Saadnajmi Saadnajmi merged commit ab72011 into microsoft:main Sep 22, 2022
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
@christophpurrer @appden
Fix resizeMode="cover" when image is resized (microsoft#1273)
62de779 
* Fix resizeMode="cover" when image is resized

Since `RCTResizeModeCover` is implemented manually on macOS (iOS benefits from `UIViewContentModeScaleAspectFill` API), we need to reprocess the image any time the bounds of the view update. The `updateImage:` method was already called on resize, so this instead stores the original image in an ivar so it can call `RCTFillImagePreservingAspectRatio` from `updateImage:` instead.

* Fix rendering template images with tintColor

This fixes a regression introduced by the previous commit, where images would no longer render with the `tintColor` on release builds. We must call `setTemplate:` (using `.template` property is illegal in Objective-C++ since `template` is reserved in C++) on the newly resized image instead of the original image.

Confirmed icons look correct in the release build of Messenger Desktop.

Co-authored-by: Scott Kyle <[email protected]>
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Mar 10, 2023
@christophpurrer @appden
Fix resizeMode="cover" when image is resized (microsoft#1273)
f092d78 
* Fix resizeMode="cover" when image is resized

Since `RCTResizeModeCover` is implemented manually on macOS (iOS benefits from `UIViewContentModeScaleAspectFill` API), we need to reprocess the image any time the bounds of the view update. The `updateImage:` method was already called on resize, so this instead stores the original image in an ivar so it can call `RCTFillImagePreservingAspectRatio` from `updateImage:` instead.

* Fix rendering template images with tintColor

This fixes a regression introduced by the previous commit, where images would no longer render with the `tintColor` on release builds. We must call `setTemplate:` (using `.template` property is illegal in Objective-C++ since `template` is reserved in C++) on the newly resized image instead of the original image.

Confirmed icons look correct in the release build of Messenger Desktop.

Co-authored-by: Scott Kyle <[email protected]>
NickGerleman added a commit to NickGerleman/react-native that referenced this pull request Apr 30, 2023
@NickGerleman @facebook-github-bot
Cleanup YGValue.h header (microsoft#1273)
0d0dd3b 
Summary:
X-link: facebook/yoga#1273

1. Simplify nan handling a bit
2. Remove string literal operators which seem dead enough to remove. These are public, so anyone could be using them, but it seems like almost nobody is.
    1. FB has no usages of `using namespace facebook::yoga::literals` (exposing the literal operators) outside of Yoga tests.
    1. There is only [a single usage](https://github.com/XITRIX/UIKit/blob/6dfba905ea83c89e255144f7ed90fde8c33ca81a/SDLTest/UIKit.hpp#L19) on GitHub.

Differential Revision: D45419970

fbshipit-source-id: 88b95a21ec643ba54e610ec57633f5dd104a30a6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harrieshin harrieshin harrieshin left review comments

@Saadnajmi Saadnajmi Saadnajmi approved these changes

Assignees

@mischreiber mischreiber

Labels
Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) Partner: Facebook
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@christophpurrer @pull-bot @Saadnajmi @harrieshin @mischreiber @appden