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(ios(old arch)): transform origin #38514

Conversation

intergalacticspacehighway
Copy link
Contributor

Summary:

This PR adds transform origin support for iOS old architecture. Will do separate PRs for android, iOS (new and old arch) to make it easy to review as mentioned in #37606 (review) by @javache. This PR also incorporates feedback/changes suggested by @javache in the original PR.

Changelog:

[IOS - old arch] [ADDED] - Transform origin

Test Plan:

Run iOS RNTester app in old architecture and test transform-origin example in TransformExample.js.

@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 Jul 19, 2023
@github-actions
Copy link

Fails
🚫

📋 Verify Changelog Format - See Changelog format

Generated by 🚫 dangerJS against 7ed9f25

@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 Jul 19, 2023
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,836,243 +453
android hermes armeabi-v7a 8,146,342 +437
android hermes x86 9,341,846 +452
android hermes x86_64 9,184,504 +443
android jsc arm64-v8a 9,449,125 +135
android jsc armeabi-v7a 8,631,149 +134
android jsc x86 9,532,025 +135
android jsc x86_64 9,775,225 +143

Base commit: 256aaa1
Branch: main

@github-actions github-actions bot added the Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue. label Jul 19, 2023
@cortinico cortinico removed the Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue. label Jul 20, 2023
@@ -111,6 +111,7 @@ const ReactNativeStyleAttributes: {[string]: AnyAttributeType, ...} = {
* Transform
*/
transform: {process: processTransform},
transformOrigin: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Use this to pre-process it on the JS side so you can share it with the Android side - https://gist.github.com/jacobp100/86bc3fa863e41f42ca091386f6252f29 - I maintain css-to-react-native so you can trust me 🤣

It gives you an array with 3 components corresponding to x, y, and z. Gives either a number ([json isKindOfClass:NSNumber.class]), or a string with a % at the end (re-use your existing logic here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, had that in mind 😅, was saving for the last. I'll check your code! Thanks!

@@ -218,12 +218,18 @@ - (RCTShadowView *)shadowView

RCT_CUSTOM_VIEW_PROPERTY(transform, CATransform3D, RCTView)
{
view.layer.transform = json ? [RCTConvert CATransform3D:json] : defaultView.layer.transform;
view.rawTransform = json;
view.layer.transform = json ? [RCTConvert CATransform3D:view.rawTransform viewWidth:view.bounds.size.width viewHeight:view.bounds.size.height transformOrigin: view.transformOrigin] : defaultView.layer.transform;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this so you can use %s in the transform property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah exactly, also center, right etc enums require height and width of the view.

@@ -785,6 +786,10 @@ - (void)reactSetFrame:(CGRect)frame
[super reactSetFrame:frame];
if (!CGSizeEqualToSize(self.bounds.size, oldSize)) {
[self.layer setNeedsDisplay];
// Update transform for transform origin due to change in view dimension
if (self.transformOrigin.length > 0) {
self.layer.transform = [RCTConvert CATransform3D:self.rawTransform viewWidth:self.bounds.size.width viewHeight:self.bounds.size.height transformOrigin: self.transformOrigin];
Copy link
Contributor

@jacobp100 jacobp100 Jul 21, 2023

Choose a reason for hiding this comment

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

I think you can use anchorPoint instead. It'd save you having to store rawTransform as a property

Note that it's a percent value rather than pixel, so if you do this you'll need to flip your logic so your non-percent values are multiplied by the view width/height. Also note that it only supports x and y, and there's anchorPointZ for the z-axis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The initial version that I tried used anchorPoint but anchorPoint also changes the position of the view and it might conflict with yoga's setting position or will need some handling so I dropped that idea, also Android has pivot x and pivot y but no pivot z so thought of using the raw translates to achieve transform-origin and keeping it consistent on android and iOS. 😅

@intergalacticspacehighway
Copy link
Contributor Author

Closing in favor of #38626

@intergalacticspacehighway intergalacticspacehighway deleted the feat/ios-old-arch-transform-origin branch August 9, 2024 22:42
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. 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.

5 participants