How to add multiple animations? about aphrodite HOT 16 CLOSED

To make it more intuitive, what if we support this as well:

StyleSheet.create({
  animated: {
    animationName: [keyframes1, keyframes2],
    animationDuration: ['2s', '1s'],
    animationIterationCount: ['1', 'infinite'],
  },
});

This could make for interesting composability models such as:

const animation1 = {
  keyframes: {/*stuff*/},
  duration: '2s',
  iterationCount: '1',
}
const animation2 = {
  keyframes: {/*stuff*/},
  duration: '1s',
  iterationCount: 'infinite',
}
StyleSheet.create({
  animated: {
    ...combineAnimations(animation1, animation2)
  },
});

Where combineAnimations could just be a util (probably best as an aphrodite plugin or something).

Though, honestly, we wouldn't need the array support to make combineAnimations work, so yeah :-)

from aphrodite.

@joshwcomeau With tests, please :)

from aphrodite.

@kitze Hmm, interesting! I didn't know you could chain animations like that. Maybe we could make it so that animationName allowed for arrays of keyframes? Then maybe you could do...

StyleSheet.create({
    animationName: [keyframes1, keyframes2],
    animationDuration: '2s, 1s',
    animationIterationCount: '1, infinite',
});

? Not super intuitive, but a really easy change to make in aphrodite.

from aphrodite.

@xymostech Ah, I see.

Let's start with what you originally proposed:

StyleSheet.create({
    animationName: [keyframes1, keyframes2],
    animationDuration: '2s, 1s',
    animationIterationCount: '1, infinite',
});

Since it looks like this feature request is not just syntactic sugar since keyframes objects like that are created specially, and allow the rest of the properties to remain as strings. If people want to build syntactic sugar helpers on top of them, they'll be welcome to.

from aphrodite.

Sounds good to me @jlfwong!

@joshwcomeau If you'd like to contribute, this one will be an easy first commit. You'll just want to modify the animationName string handler to also accept arrays, and combine them appropriately: https://github.com/Khan/aphrodite/blob/master/src/inject.js#L80 (the fontFamily string handler also handles arrays, so you can look at what it does if you're confused) If you don't have time I'll get to this next week.

from aphrodite.

What about:

{
  animation: 'first-animation 2s infinite, another-animation 1s'
}

in your StyleSheet.create call?

from aphrodite.

Yeah but if the keyframes are created like this:

const keyframes = {
    'from': {
      opacity: 0,
      transform: `translate3d(0, 2000px, 0) scale(-0.01)`
    },
    'to': {
      opacity: 1,
      transform: 'none',
    }
 };

You can only write:

myElement: {
    animationName: keyframes
}

Then aphrodite will process that as animation: keyframes_93713mfjd and it will include the keyframes object (as css).

If you write:

myElement: {
    animation: `${keyframes} 1s infinite`
}

It will not include the keyframes as css, and it will produce: animation: Object [Object] 1 infinite

from aphrodite.

Here's the PR, it has TODO that should implement this.

a74a1ac#diff-16ceeedcc8c01ce4cb6aadc71661562aR457

from aphrodite.

That would definitely be better! combineAnimations could even know that the default iterationCount is 1 and stuff.

from aphrodite.

Any update on this? I need it for a project I'm working on.

Happy to create a PR, but I wanna make sure 1) It's still desired and 2) it isn't being worked on.

Also, any pointers as to desired implementation would be great :)

from aphrodite.

I'm a little torn about whether this lives in core or not. This is arguable syntactic sugar, but the non-sugary version is pretty obnoxious and would be annoying to maintain, and it seems like there's one correct solution... The combineAnimations method without adding array support appeals to me.

I think it makes sense to be part of core, but don't have strong feels. Thoughts, @xymostech?

from aphrodite.

@jlfwong I would prefer to have the array support, and spin off combineAnimations into a separate plugin. I think I prefer that because it would give people more control over how they can interact with Aphrodite if they want to build more complex abstractions over our API, whereas forcing people to go through combineAnimations would make that harder. Yes it's obnoxious for the general user, but they can just use combineAnimations, and someone who wants more control gets it? Maybe that's not a good argument; we might want to force people to use our tooling so we have more control over the API.

I can put up a PR for adding the array support unless you want to, @joshwcomeau (should it add array support for just animationName or all of the animation properties?), and we can play around with what a combineAnimations plugin would look like...

from aphrodite.

@xymostech My gripe with the arrays support is that the behavior is unclear, IMO, when there are multiple different lengths of arrays.

e.g. what does this mean?

StyleSheet.create({
  animated: {
    animationName: [keyframes1, keyframes2],
    animationDuration: ['2s'],
    animationIterationCount: ['1', 'infinite', '1'],
  },
});

I guess we could enforce that all of them are the same length and complain loudly if they're not?

from aphrodite.

@xymostech I think it should be for all properties. IMO the benefit to multiple animations is that they can happen at different speeds. For example, if you want to fade something in as it grows, but want the fade to happen faster:

const growKeyframes = {
  from: {
    transform: 'scale(0)',
  },
  to: {
    transform: 'scale(1)',
  },
};

const fadeKeyframes = {
  from: {
    opacity: 0,
  },
  to: {
    opacity: 1,
  },
};

const styles = StyleSheet.create({
  growAndFade: {
    animationName: [fadeKeyframes, growKeyframes],
    animationDuration: ['250ms', '1000ms'],
  },
});

@jlfwong yeah, I don't see any way other than complaining. We could use default values for end gaps, but agree it's super unclear that way.

I had another idea. I haven't dug into the code much, so I don't know how feasible it is, but what about supporting the short-hand property as a 2D array?

const styles = StyleSheet.create({
  growAndFade: {
    animation: [
      ['250ms', fadeKeyframes],
      ['1000ms', growKeyframes],
    ],
  },
});

It would be very clunky to enforce two arrays for a single property, though, so it could also support a single animation as a single array:

const styles = StyleSheet.create({
  growAndFade: {
    animation: ['250ms', fadeKeyframes],
  },
});

These are just ideas off the top of my head, and could be problematic for reasons I haven't yet anticipated. Really, any of these ideas would make me a happy camper.

As for actually doing the PR, I'm happy either way. I do like the idea of contributing to Aphrodite though :)

from aphrodite.

My gripe with the arrays support is that the behavior is unclear, IMO, when there are multiple different lengths of arrays.

It'll be clear what markup we should generate though, right? Maybe I'm misunderstanding, but I think we would just be taking the array elements and putting commas in between them. Having different length arrays isn't a problem from Aphrodite's point of view (that is, we can still generate valid markup), it just won't make semantic sense. I think that is fine. (we let people generate semantically incorrect markup in other places too, and it's still the user's fault) Thoughts?

@joshwcomeau We would actually need for the animationName property to be an array, since we do something special with the animations you give there. For the other properties, you could get away with just generating the string '2s, 1s' yourself, so we wouldn't need for them to accept arrays. Presumably the combineAnimations helper would handle this for you so it wouldn't be a problem. Just trying to have the minimum number of "special" properties to make the API less confusing.

from aphrodite.

Was indeed very straightforward - PR has been submitted!

Let me know if any changes are required :)

from aphrodite.