react-native icon indicating copy to clipboard operation
react-native copied to clipboard

feat(iOS/fabric): percentage support in translate

Open intergalacticspacehighway opened this issue 2 years ago • 15 comments

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 avatar Mar 25 '24 12:03 NickGerleman

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

NickGerleman avatar Mar 27 '24 20:03 NickGerleman

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

analysis-bot avatar Mar 30 '24 11:03 analysis-bot

@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 avatar Apr 15 '24 19:04 NickGerleman

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

cortinico avatar Apr 22 '24 09:04 cortinico

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

cortinico avatar Apr 23 '24 11:04 cortinico

I'll import this and see how it looks

NickGerleman avatar May 01 '24 00:05 NickGerleman

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar May 01 '24 00:05 facebook-github-bot

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

NickGerleman avatar May 02 '24 18:05 NickGerleman

Tests pass!

Small changes I needed to make:

  1. Formatting
  2. A shadowing warning the OSS build didn't complain as loudly about
  3. Remove getValueUnitFromRawValue() in favor of fromRawValue overload we added recently. For dependency organization purposes, ValueUnit should not depend on RawValue

NickGerleman avatar May 02 '24 19:05 NickGerleman

@NickGerleman merged this pull request in facebook/react-native@f997b81288148b4eee30b65375059feac2a14cd8.

facebook-github-bot avatar May 17 '24 02:05 facebook-github-bot

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?

github-actions[bot] avatar May 17 '24 02:05 github-actions[bot]

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.

NickGerleman avatar May 20 '24 10:05 NickGerleman