Skip to content

feat: Add string support to the transform property - #34660

Closed
gabrieldonadel wants to merge 4 commits into
react:mainfrom
gabrieldonadel:feat/add-string-support-to-transform
Closed

feat: Add string support to the transform property#34660
gabrieldonadel wants to merge 4 commits into
react:mainfrom
gabrieldonadel:feat/add-string-support-to-transform

Conversation

@gabrieldonadel

Copy link
Copy Markdown
Collaborator

Summary

This updates the transform property to support string values as requested on #34425. This also updates the existing unit tests of the processTransform function ensuring the style processing works as expected and updates the TransformExample on RNTester in order to facilitate the manual QA of this.

Changelog

[General] [Added] - Add string support to the transform property

Test Plan

  1. Open the RNTester app and navigate to the Transforms page
  2. Check the transform style through the Transform using a string section
Screen.Recording.2022-09-11.at.18.57.17.mov

@facebook-github-botfacebook-github-bot added 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. labels Sep 11, 2022
@react-native-botreact-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Sep 11, 2022
@gabrieldonadel
gabrieldonadelforce-pushed the feat/add-string-support-to-transform branch from a9073f3 to 7484153CompareSeptember 11, 2022 22:20
@analysis-bot

analysis-bot commented Sep 11, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a7,644,136-37,477
androidhermesarmeabi-v7a7,056,169-30,204
androidhermesx867,945,632-39,038
androidhermesx86_647,917,550-40,026
androidjscarm64-v8a9,516,313-37,028
androidjscarmeabi-v7a8,291,915-29,692
androidjscx869,455,641-38,506
androidjscx86_6410,046,708-39,376

Base commit: bfb36c2
Branch: main

@analysis-bot

analysis-bot commented Sep 11, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/a--

Base commit: bfb36c2
Branch: main

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

Comment threadLibraries/StyleSheet/__tests__/processTransform-test.js Outdated
Comment threadLibraries/StyleSheet/__tests__/processTransform-test.js Outdated
Comment threadLibraries/StyleSheet/__tests__/processTransform-test.js Outdated
Comment threadLibraries/StyleSheet/__tests__/processTransform-test.js Outdated
Comment threadLibraries/StyleSheet/__tests__/processTransform-test.js Outdated
@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

| {|+translateY: number | AnimatedNode|}
| {|
+translate:
| [number | AnimatedNode, number | AnimatedNode]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The tests for existing RN syntax include 3 args, but noticed that doesnt seem to be in the type def.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yeah, actually I couldn't find any reference to the translate param in the official docs https://reactnative.dev/docs/next/transforms#transform, maybe it's something used internally?

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@react-native-bot

Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @gabrieldonadel in 34db2d4.

When will my fix make it into a release? | Upcoming Releases

@react-native-botreact-native-bot added the Merged This PR has been merged. label Sep 22, 2022
@gabrieldonadel
gabrieldonadel deleted the feat/add-string-support-to-transform branch September 22, 2022 14:37
@kelset

Copy link
Copy Markdown
Contributor

@gabrieldonadel / @necolas I just tested in 0.71 branch and in RNTester, when I try to the test page, it crashes:
Screenshot 2022-11-09 at 17 14 44

@gabrieldonadel

Copy link
Copy Markdown
CollaboratorAuthor

@gabrieldonadel / @necolas I just tested in 0.71 branch and in RNTester, when I try to the test page, it crashes

@kelset seems that I forgot to update [0] on RN-Tester, gonna open a PR in few updating that to use px

[0] https://github.com/gabrieldonadel/react-native/blob/8d17c5e7cac50279fc92b9de29b6cbe2036cc06c/packages/rn-tester/js/examples/Transform/TransformExample.js#L211

@acoates-ms

Copy link
Copy Markdown
Contributor

I hit this when integrating 0.71 into react-native-windows. Our automation hit this crash, so I've had to manually disable this test page in our tests for now. The main thing I want to be sure of is that any transform value that was valid before this change is still valid. -- Maybe we want to enforce units on transforms going forward, but failures there should be yellowboxes not invariants during the transition period. Otherwise, it will be very hard for any package that wants to support multiple versions of RN.

@gabrieldonadel

Copy link
Copy Markdown
CollaboratorAuthor

The main thing I want to be sure of is that any transform value that was valid before this change is still valid.

That shouldn't be a problem as we're only requiring units for string values, everything that was valid before is still going to be valid.

@gabrieldonadel

Copy link
Copy Markdown
CollaboratorAuthor

Thanks for the report @kelset, I've just opened a PR fixing this #35292.

This also fixes your problem @acoates-ms

@necolas

Copy link
Copy Markdown

failures there should be yellowboxes not invariants during the transition period

Yes I think invalid values should be yellowboxes, mostly because soft failures with warnings are better for styling than crashes. But we still shouldn't allow invalid syntax/values to work as if it were valid

facebook-github-bot pushed a commit that referenced this pull request Nov 9, 2022
Summary:
As pointed out by kelset on #34660 (comment) accessing the `TransformExample` is currently crashing the `RNTester` app due to missing units in one of the transformers. This PR fixes it by updating the transform value to a valid string.
## Changelog
[Internal] [Fixed] - Add missing translate units to `RNTester` `TransformExample`
Pull Request resolved: #35292
Test Plan:
1. Open the RNTester app and navigate to the Transforms page
2. Check the transform style through the `Transform using a string` section
https://user-images.githubusercontent.com/11707729/200916399-779b2eeb-2bd8-4642-97a3-f050d6dd4278.mov
Reviewed By: christophpurrer
Differential Revision: D41164541
Pulled By: necolas
fbshipit-source-id: 4aa62980001a6f8ccf0108cb3af1e573b67e02b1
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Needs TypeScript UpdateShared with MetaApplied via automation to indicate that an Issue or Pull Request has been shared with the team.Type: EnhancementA new feature or enhancement of an existing feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@gabrieldonadel@analysis-bot@facebook-github-bot@react-native-bot@kelset@acoates-ms@necolas@lunaleaps