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: animated component style prop accepts transform functions #782

Merged
merged 8 commits into from
Aug 16, 2019

Conversation

dbismut
Copy link
Contributor

@dbismut dbismut commented Aug 12, 2019

Description

This pull requests allows passing transform functions to the style prop of a web animated component.

Before

Currently, most web-based animations deal with the transform style attribute. Because transform is a string composed of transform functions, it was necessary to interpolate AnimatedValue as in:

const { scale } = useSpring({ scale: toggle ? 1.5 : 1 })
<animated.div style={{ transform: scale.to(s => `scale(${s})`) }} />

Or even more cumbersome:

const { x, scale } = useSpring(toggle ? { scale: 1.5, x: 10 } : { scale: 1, x: 0 })
<animated.div
  style={{ transform: to([scale, x], (s, x) => `transform3d(${x}px,0,0) scale(${s})`) }}
/>

After

The style prop would accept all transform functions (with the exception of perspective that would conflict with the native style attribute of the same name), plus x, y, z as shortcuts for translate3d(x,y,z). Moreover, default px and deg units would be added to unit-less values.

const { x, scale } = useSpring(toggle ? { scale: 1.5, x: 10 } : { scale: 1, x: 0 })
<animated.div style={{ x, scale }} />

Just like other style attributes, transform functions can accept numbers, string, arrays, AnimatedValue or AnimatedArray.

Moreover, if the overall transform is the identity the animated component will actually get a transform: none, which is necessary for fixed children to behave correctly.

<animated.div style={{ x: 0, y: 0, scale: 1, rotate: 0 }} />
// will result in the following markup
<div style="transform: none;"></div>

What remains to be done

Write test files!

@drcmda drcmda requested a review from aleclarson August 12, 2019 23:33
@drcmda
Copy link
Member

drcmda commented Aug 12, 2019

looks great imo

Copy link
Contributor

@aleclarson aleclarson left a comment

Choose a reason for hiding this comment

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

Nice work! Just need to add more tests and get the current test to actually run (there's a transpiling issue with Jest). I got types working, and cleaned up a little. :)

targets/web/src/AnimatedStyle.ts Outdated Show resolved Hide resolved
targets/web/src/AnimatedStyle.ts Show resolved Hide resolved
@aleclarson aleclarson force-pushed the feat/style-with-transforms branch from a7b7db7 to 04be6b3 Compare August 13, 2019 14:15
@dbismut
Copy link
Contributor Author

dbismut commented Aug 13, 2019

@aleclarson I think I've fixed the transpiling issue, now animated.test.tsx properly runs through tests... but fails.

@dbismut
Copy link
Contributor Author

dbismut commented Aug 13, 2019

Ok, now at least in my case, all tests but the one that concerns this PR pass properly.

@aleclarson
Copy link
Contributor

More tests needed:

  • pass some with units (eg: "0px") & some without
  • array value (eg: translate: [1, '1px'])
  • combine multiple transform keys
    • scale + transform
    • x + y + z + rotate
  • identity transform (eg: scale: [1, 1] and rotate3d: [1, 1, 1, 0])

@dbismut
Copy link
Contributor Author

dbismut commented Aug 15, 2019

Will do this tonight. I already feel there’s an issue with rotate3d which in the current implementation would add unit defaults to all elements of the array when only the last element of the array should have one. Will also fix this.

@aleclarson
Copy link
Contributor

Does matrix have default unit applied to each number? It's not supposed to, I think.

@dbismut
Copy link
Contributor Author

dbismut commented Aug 15, 2019

I don't think there's any unit that would be added to matrix would it? Do you want me to add a test?

@aleclarson
Copy link
Contributor

aleclarson commented Aug 15, 2019

Yeah, add a test please. There's no special case like you added for rotate3d in 6fb00aa, so I bet it's broken! Edit: Ah nevermind, I forgot that pxDefaults and dgDefaults determine whether units are added. My bad!

Also, I don't think 286d21e and de22f80 are necessary. What made you think they were? The AnimatedProps type adds support for Animated instances automatically, except using the SpringValue type.

@dbismut
Copy link
Contributor Author

dbismut commented Aug 15, 2019

Yeah, add a test please. There's no special case like you added for rotate3d in 6fb00aa, so I bet it's broken!

Sure I can add a test, but matrix behaves like scale which doesn't have a default unit either and has no special case. The reason why rotate3d is special is because the first three elements of the array shouldn't have a default unit, while the fourth one should. This is a special case since for other keys that support arrays (ie transform) we add the default unit to every items of the array.

Also, I don't think 286d21e and de22f80 are necessary. What made you think they were? The AnimatedProps type adds support for Animated instances automatically, except using the SpringValue type.

Hm Number and String types are superfluous you're right, but if I remove AnimatedArray I get a warning in the console.

Type '{ scale: AnimatedValue<number>; translate: AnimatedArray; translate3d: [AnimatedValue<number>, AnimatedValue<number>, string]; }' is not assignable to type '{ clipPath?: string | SpringValue<string> | undefined; filter?: string | SpringValue<string> | undefined; marker?: string | SpringValue<string> | undefined; mask?: string | number | SpringValue<...> | undefined; ... 768 more ...; matrix3d?: { ...; } | ... 1 more ... | undefined; }'.
  Types of property 'translate' are incompatible.
    Type 'AnimatedArray' is not assignable to type 'string | number | SpringValue<string | number> | [string | number | SpringValue<string | number>, string | number | SpringValue<string | number>] | undefined'.
      Type 'AnimatedArray' is not assignable to type 'SpringValue<string | number>'.
        Types of property 'getValue' are incompatible.
          Type '(animated?: boolean | undefined) => any[]' is not assignable to type '() => string | number'.
            Type 'any[]' is not assignable to type 'string | number'.
              Type 'any[]' is not assignable to type 'string'.

@aleclarson
Copy link
Contributor

aleclarson commented Aug 15, 2019

Hm Number and String types are superfluous you're right, but if I remove AnimatedArray I get a warning in the console.

The public API never exposes AnimatedArray to the user. You'll want to cast that to a SpringValue instead. (Though, it looks like AnimatedProps is slightly broken, since it allows [SpringValue<number>, SpringValue<number>] but not SpringValue<[number, number]>)

@dbismut
Copy link
Contributor Author

dbismut commented Aug 16, 2019

I'm not a TS expert, I'm not exactly sure what I should do Alec, sorry... Are you saying I should replace AnimatedArray with SpringValue in the type file animated.ts, or/and remove AnimatedArray from the test file animated.test.tsx?

dbismut and others added 8 commits August 16, 2019 14:17
Shortcut props include:
- x/y/z
- translate/translateX/translateY/translate3d
- rotate/rotateX/rotateY/rotate3d
- scale/scaleX/scaleY/scale3d
- skew/skewX/skewY
- matrix/matrix3d

These props and the "transform" prop are combined (in order of appearance) into a single "transform" string.

This feature is for "@react-spring/web" only.

Co-Authored-By: Alec Larson <[email protected]>
Co-Authored-By: Paul Henschel <[email protected]>
@aleclarson aleclarson force-pushed the feat/style-with-transforms branch from e749ad3 to 5ce760c Compare August 16, 2019 18:20
@aleclarson aleclarson merged commit 6406466 into v9 Aug 16, 2019
@aleclarson aleclarson deleted the feat/style-with-transforms branch August 16, 2019 18:21
@aleclarson
Copy link
Contributor

Great work, @dbismut! 🎉

@dbismut
Copy link
Contributor Author

dbismut commented Aug 16, 2019

A lot was done by @drcmda but I’m happy to take credit for him 😅

@VincentCtr
Copy link

Hello guys,

It seems that this feature does not work with the property transformOrigin: 'bottom center'

It creates a regression as it does not work also with the old syntax.

@drcmda
Copy link
Member

drcmda commented Aug 29, 2019

@dbismut

@dbismut
Copy link
Contributor Author

dbismut commented Aug 29, 2019

@drcmda hopefully #796 should fix this. Good catch @VincentCtr!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants