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

fix: [TS] Update translateX & translateY types to support % #42671

Closed

Conversation

retyui
Copy link
Contributor

@retyui retyui commented Jan 25, 2024

Summary:

After update to the latest react-native version
we discover that we are unable to use number% value for translate* props :

StyleSheet.create({
  root: {
    transform: [
      { translateX: '-50%'  }, 
      // ^^^^^^ TS Error: Type string is not assignable to type AnimatableNumericValue | undefined
    ],
  }
});

percentage values are supported, demo: https://snack.expo.dev/@retyui/test-tstransform

Changelog:

[GENERAL] [FIXED] - Update typescript definition of translateX & translateX to be able to use percentage values

Test Plan:

yarn tsc --noEmit

@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 Jan 25, 2024
@mdvacca
Copy link
Contributor

mdvacca commented Jan 25, 2024

@NickGerleman can you review this?

@NickGerleman
Copy link
Contributor

@retyui thanks for the change!

I took a look at StyleSheetTypes.js as the source of truth for these. Flow doesn't have template literal types, but it looks like we probably also want to make this same change for rotate and skew.

  transform?:
    | $ReadOnlyArray<
        | {|+perspective: number | AnimatedNode|}
        | {|+rotate: string | AnimatedNode|}
        | {|+rotateX: string | AnimatedNode|}
        | {|+rotateY: string | AnimatedNode|}
        | {|+rotateZ: string | AnimatedNode|}
        | {|+scale: number | AnimatedNode|}
        | {|+scaleX: number | AnimatedNode|}
        | {|+scaleY: number | AnimatedNode|}
        | {|+translateX: number | AnimatedNode|}
        | {|+translateY: number | AnimatedNode|}
        | {|
            +translate:
              | [number | AnimatedNode, number | AnimatedNode]
              | AnimatedNode,
          |}
        | {|+skewX: string | AnimatedNode|}
        | {|+skewY: string | AnimatedNode|}
        // TODO: what is the actual type it expects?
        | {|
            +matrix: $ReadOnlyArray<number | AnimatedNode> | AnimatedNode,
          |},
      >
    | string,

@retyui
Copy link
Contributor Author

retyui commented Jan 26, 2024

@NickGerleman

but it looks like we probably also want to make this same change for rotate and skew.

No, rotate and skew allow to pass any string because use AnimatableStringValue = string | Animated.AnimatedNode

interface SkewXTransform {
skewX: AnimatableStringValue;
}
interface SkewYTransform {
skewY: AnimatableStringValue;
}

interface RotateTransform {
rotate: AnimatableStringValue;
}
interface RotateXTransform {
rotateX: AnimatableStringValue;
}
interface RotateYTransform {
rotateY: AnimatableStringValue;
}
interface RotateZTransform {
rotateZ: AnimatableStringValue;
}

@facebook-github-bot
Copy link
Contributor

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

Copy link

This pull request was successfully merged by @retyui in b133bf6.

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

@github-actions github-actions bot added the Merged This PR has been merged. label Jan 30, 2024
@burakgormek
Copy link

case 'translateX':
case 'translateY':
case 'scale':
case 'scaleX':
case 'scaleY':
invariant(
typeof value === 'number',
'Transform with key of "%s" must be a number: %s',
key,
stringifySafe(transformation),
);

I think percentage is not supported based on processTransform 🤔 or do we need to update processTransform?

@retyui
Copy link
Contributor Author

retyui commented Jan 31, 2024

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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants