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: Add string support to the transform property #34660

Conversation

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

analysis-bot commented Sep 11, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,644,136 -37,477
android hermes armeabi-v7a 7,056,169 -30,204
android hermes x86 7,945,632 -39,038
android hermes x86_64 7,917,550 -40,026
android jsc arm64-v8a 9,516,313 -37,028
android jsc armeabi-v7a 8,291,915 -29,692
android jsc x86 9,455,641 -38,506
android jsc x86_64 10,046,708 -39,376

Base commit: bfb36c2
Branch: main

@analysis-bot
Copy link

analysis-bot commented Sep 11, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: bfb36c2
Branch: main

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
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
Contributor

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
Collaborator Author

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
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
Collaborator

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

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

@react-native-bot react-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
Contributor

kelset commented Nov 9, 2022

@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
Collaborator Author

@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
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
Collaborator Author

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
Collaborator Author

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

This also fixes your problem @acoates-ms

@necolas
Copy link
Contributor

necolas commented Nov 9, 2022

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
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This updates the `transform` property to support string values as requested on facebook#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

Pull Request resolved: facebook#34660

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/189550548-ee3c14dd-11c6-4fd1-bd74-f6b52ecb9eae.mov

Reviewed By: lunaleaps

Differential Revision: D39423409

Pulled By: cipolleschi

fbshipit-source-id: 0d7b79178eb33f34ae55a070ce094360b544361f
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
As pointed out by kelset on facebook#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: facebook#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 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. Merged This PR has been merged. Needs TypeScript Update Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants