-
Notifications
You must be signed in to change notification settings - Fork 24k
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(android): percentage support in translate #43193
feat(android): percentage support in translate #43193
Conversation
Base commit: 12f2c3c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! A few questions.
@@ -177,18 +177,15 @@ public void setBackgroundColor(@NonNull T view, int backgroundColor) { | |||
public void setTransform(@NonNull T view, @Nullable ReadableArray matrix) { | |||
view.setTag(R.id.transform, matrix); | |||
view.setTag(R.id.invalidate_transform, true); | |||
view.addOnLayoutChangeListener(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably discussed this in the original transformOrigin PR, but why can't we override layout
(or as Android prefers onLayout
) to update the transform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah i remember discussing it here - #38558 (comment). To override layout
i think we'll have to move transform logic to ReactViewGroup
.
onLayout
runs during layout pass and can be used to update child view positions, in our case we update transform of the view. Not very ideal, i guess in the long run moving it to RootViewGroup
might be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked code, and Android will no-op if adding existing listener, but it's a little bit smelly that we can add ourselves as a listener, after we have already added ourselves as listener. Not sure a better way though (probably don't want to waste cycles to always attach).
Co-authored-by: Pieter De Baets <pieter.debaets@gmail.com>
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. <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: #43192 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 - #43193, #43191 Reviewed By: javache Differential Revision: D56802425 Pulled By: NickGerleman fbshipit-source-id: 978cbbdde004afe1e68ffee9a3c7eb7d16336b46
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of minor comments, but this seems pretty nicely scoped 👍
@@ -177,18 +177,15 @@ public void setBackgroundColor(@NonNull T view, int backgroundColor) { | |||
public void setTransform(@NonNull T view, @Nullable ReadableArray matrix) { | |||
view.setTag(R.id.transform, matrix); | |||
view.setTag(R.id.invalidate_transform, true); | |||
view.addOnLayoutChangeListener(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked code, and Android will no-op if adding existing listener, but it's a little bit smelly that we can add ourselves as a listener, after we have already added ourselves as listener. Not sure a better way though (probably don't want to waste cycles to always attach).
@@ -171,7 +171,7 @@ public void onLayoutChange( | |||
if ((currentHeight != oldHeight || currentWidth != oldWidth)) { | |||
ReadableArray transformOrigin = (ReadableArray) v.getTag(R.id.transform_origin); | |||
ReadableArray transformMatrix = (ReadableArray) v.getTag(R.id.transform); | |||
if (transformMatrix != null && transformOrigin != null) { | |||
if (transformMatrix != null || transformOrigin != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a matrix most of the time, right? Can we rename?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, renamed to transforms
since it's an array.
@@ -130,6 +152,14 @@ public static void processTransform( | |||
} | |||
} | |||
|
|||
private static double parsePercentageToValue(String stringValue, double dimension) { | |||
if (stringValue.endsWith("%")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the old code was reliable here, but we usually want to avoid native crashes on invalid style values. So we probably want to be handling NumberFormatException
somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, updated!
i moved it in onAfterUpdateTransaction so it gets called a bit less and yes duplicate calls for same listener will be ignored. But i agree this is not very optimal. If all views are getting extended from |
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@NickGerleman merged this pull request in c13790f. |
This pull request was successfully merged by @intergalacticspacehighway in c13790f. When will my fix make it into a release? | How to file a pick request? |
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. <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: facebook#43192 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 - facebook#43193, facebook#43191 Reviewed By: javache Differential Revision: D56802425 Pulled By: NickGerleman fbshipit-source-id: 978cbbdde004afe1e68ffee9a3c7eb7d16336b46
Summary: This PR adds percentage support in translate properties for android. Isolating this PR for easier reviews. ## Changelog: [Android] [ADDED] - Percentage support in translate <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: facebook#43193 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 - facebook#43191, facebook#43192 Reviewed By: joevilches Differential Revision: D57723216 Pulled By: NickGerleman fbshipit-source-id: c9da007678341b62745df858f043821bcc662a98
Summary:
This PR adds percentage support in translate properties for android. Isolating this PR for easier reviews.
Changelog:
[Android] [ADDED] - Percentage support in translate
Test Plan:
processTransform-test.js
. The regex is not perfect (values like 20px%, 20%px will pass, can be improved, let me know!)Related PRs - #43191, #43192