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

How to add multiple animations? #78

Closed
kitze opened this issue May 26, 2016 · 16 comments · Fixed by #167
Closed

How to add multiple animations? #78

kitze opened this issue May 26, 2016 · 16 comments · Fixed by #167

Comments

@kitze
Copy link

@kitze kitze commented May 26, 2016

The shorthand syntax for animations doesn't work and i would like to have this:

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

Is it possible?

@sophiebits

This comment has been minimized.

Copy link

@sophiebits sophiebits commented May 26, 2016

What about:

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

in your StyleSheet.create call?

@kitze

This comment has been minimized.

Copy link
Author

@kitze kitze commented May 26, 2016

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

@kitze

This comment has been minimized.

Copy link
Author

@kitze kitze commented May 26, 2016

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

a74a1ac#diff-16ceeedcc8c01ce4cb6aadc71661562aR457

@xymostech

This comment has been minimized.

Copy link
Contributor

@xymostech xymostech commented May 27, 2016

@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.

@kentcdodds

This comment has been minimized.

Copy link
Contributor

@kentcdodds kentcdodds commented May 27, 2016

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 :-)

@xymostech

This comment has been minimized.

Copy link
Contributor

@xymostech xymostech commented May 27, 2016

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

@joshwcomeau

This comment has been minimized.

Copy link
Contributor

@joshwcomeau joshwcomeau commented Nov 4, 2016

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 :)

@jlfwong

This comment has been minimized.

Copy link
Collaborator

@jlfwong jlfwong commented Nov 4, 2016

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?

@xymostech

This comment has been minimized.

Copy link
Contributor

@xymostech xymostech commented Nov 4, 2016

@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...

@jlfwong

This comment has been minimized.

Copy link
Collaborator

@jlfwong jlfwong commented Nov 4, 2016

@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?

@joshwcomeau

This comment has been minimized.

Copy link
Contributor

@joshwcomeau joshwcomeau commented Nov 4, 2016

@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 :)

@xymostech

This comment has been minimized.

Copy link
Contributor

@xymostech xymostech commented Nov 4, 2016

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.

@jlfwong

This comment has been minimized.

Copy link
Collaborator

@jlfwong jlfwong commented Nov 5, 2016

@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.

@xymostech

This comment has been minimized.

Copy link
Contributor

@xymostech xymostech commented Nov 5, 2016

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.

@jlfwong

This comment has been minimized.

Copy link
Collaborator

@jlfwong jlfwong commented Nov 5, 2016

@joshwcomeau With tests, please :)

@joshwcomeau

This comment has been minimized.

Copy link
Contributor

@joshwcomeau joshwcomeau commented Nov 6, 2016

Was indeed very straightforward - PR has been submitted!

Let me know if any changes are required :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.