feat(iOS/fabric): percentage support in translate
Summary:
This PR adds percentage support in translate properties for new arch iOS. Isolating this PR for easier reviews.
The approach taken here introduces usage of ValueUnit struct for transform operations so it can support % in translates and delay the generation of actual transform matrix until view dimensions are known. I have tried to keep the changes minimal and reuse existing APIs, open to changes if there's an alternative approach.
Changelog:
[IOS] [ADDED] - Percentage support in translate in new arch.
Test Plan:
- Checkout TransformExample.js -> Translate percentage example.
- Added a simple test in
processTransform-test.js. The regex is not perfect (values like 20px%, 20%px will pass, can be improved, let me know!)
Related PRs - https://github.com/facebook/react-native/pull/43193, https://github.com/facebook/react-native/pull/43191
@javache @intergalacticspacehighway anything pending here?
@NickGerleman i got occupied with stuff at work 😅. @javache has commented in other PRs too. I am gonna wrap it up this week.
No worries! I will take a look again after next revision.
| Platform | Engine | Arch | Size (bytes) | Diff |
|---|---|---|---|---|
| android | hermes | arm64-v8a | 19,427,379 | +16,635 |
| android | hermes | armeabi-v7a | n/a | -- |
| android | hermes | x86 | n/a | -- |
| android | hermes | x86_64 | n/a | -- |
| android | jsc | arm64-v8a | 22,800,763 | +16,493 |
| android | jsc | armeabi-v7a | n/a | -- |
| android | jsc | x86 | n/a | -- |
| android | jsc | x86_64 | n/a | -- |
Base commit: 03a51da727022d96f7f9bbc0840b1409121d0429 Branch: main
@NickGerleman @javache Any pointers or command how can i run these transform tests?
I'll add some tests here for translate percentage.
@NickGerleman @javache Any pointers or command how can i run these transform tests?
I'll add some tests here for translate percentage.
I think @cortinico wired this up to Android CMake build. I think this is called by Gradle inc in OSS CI, but I haven't run myself. https://github.com/facebook/react-native/blob/6dd83cbb3510b204974d4f9876a789b127e1bfef/packages/react-native/ReactAndroid/src/main/jni/CMakeLists.txt#L141
@NickGerleman @javache added a testcase. CI is fine, so hopefully the test is running? Seems to be ready to take another look at this and the other PRs 🙏
I think @cortinico wired this up to Android CMake build. I think this is called by Gradle inc in OSS CI, but I haven't run myself.
@intergalacticspacehighway Sadly those tests don't run automatically in OSS as we don't have a running device on CI. They will execute when we import the PR internally though so it's good that you added one 👍
Thanks @cortinico Any way i can run these tests locally, just to confirm? i can see it bundles the cpp test files in android local build. But i couldn't find a way to run them. Is it like a separate project/gradle-task?
Thanks @cortinico Any way i can run these tests locally, just to confirm? i can see it bundles the cpp test files in android local build. But i couldn't find a way to run them. Is it like a separate project/gradle-task?
Instructions on how to run them are here: https://github.com/ShiraOzeri/Android-NDK-with-Google-Test#android-ndk-with-google-test Just note that we don't run those in OSS so they might just be broken, but once we import it internally we can tell if they're working or not
I'll import this and see how it looks
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
Something seems off. FB internal tests are running indefinitely 😅
Something seems off. FB internal tests are running indefinitely 😅
Needs some build fixes for Buck. Going to see if I can take a look at this today
Tests pass!
Small changes I needed to make:
- Formatting
- A shadowing warning the OSS build didn't complain as loudly about
- Remove
getValueUnitFromRawValue()in favor of fromRawValue overload we added recently. For dependency organization purposes,ValueUnitshould not depend onRawValue
@NickGerleman merged this pull request in facebook/react-native@f997b81288148b4eee30b65375059feac2a14cd8.
This pull request was successfully merged by @intergalacticspacehighway in f997b81288148b4eee30b65375059feac2a14cd8.
When will my fix make it into a release? | How to file a pick request?
cc - @NickGerleman Thanks for reviewing. fyi two more PRs are there - https://github.com/facebook/react-native/pull/43193 https://github.com/facebook/react-native/pull/43191 🙏
cc - @NickGerleman Thanks for reviewing. fyi two more PRs are there - #43193 #43191 🙏
I will try to take a closer look at Android one soon. In general we have been recently wanting to avoid non-trivial Paper-specific changes (though for Android it is kinda shared), so the iOS one might make less sense.