Any way to specify custom animations?

[react-native-modal]

Any way to specify custom animations?

[iRoachie]

Sure thing @mmazzarolo

[mmazzarolo]

Hey @Nickersoft, thank you for opening the issue.
Since we don't have any platform check inside the modal, my first guess is that the issue might be somewhat related to react-native-animatable.
Maybe your initializeRegistryWithDefinitions() call conflicts with our internal call here? 🤔

[Nickersoft]

@mmazzarolo Yeah, I noticed that.. and actually, after running my code on Android then switching back to my iOS emulator, I was getting the same error there too. It almost seems like the only reason it was working before was a race condition of some sort (maybe my animations were the most recent to register?)

I was actually giving thought to adding custom animation support myself... however, I figured I'd start a discussion on what the API for that would look like. Passing in your own animation registry seems messy, plus I'm not even sure what you'd call that prop. It would be nice if you could pass in either the string of an existing animation, or an animation object like you would find in an initializeRegistryWithDefinitions() call. That said, this library's code already initializes the registry with custom animations so I'm not even sure what kind of refactoring would be required to make that happen. Either way, it's an important functionality for my app to have, so I'm determined to try to find a solution.

What are your thoughts on this?

[mmazzarolo]

@Nickersoft glad you're interested in sending a PR!

I'll admit I'm not an expert of react-native-animatable, but I'll try to give you as much supported as I can.
First of all, the call to initializeRegistryWithDefinitions() was added in PR #17 to fix a breaking change of the sliding animation in a react-native-animatable update.

Regarding the prop to customize the animation: I'm not too keen on adding too many customization options on the component.
Also, I still haven't put enough resources in learning how initializeRegistryWithDefinitions() works, but the more I think about it, the more I think it should stay outside of the modal definition (since it initializes the registry globally, am I right?).
Maybe showing in the modal README.md how to add a properly working slide animation pointing it to react-native-animatable docs might be enough?

...Still, in the meanwhile, considering that we already use the initializeRegistryWithDefinitions(), I guess it won't hurt adding a way to customize it too, so go for it. A prop name like animatableDefinitions should be good, what do you think?

You would also need to move the initialization somewhere in the modal lifecycle, making sure that it is called with the current default definitions if animatableDefinitions is not provided and it is backward compatible 👍

[mmazzarolo]

Please note that if you're forking from master, there is still PR #65 that has not been published on npm (it should work thought, since it has been tested).

Also, I don't know if you're interested, but if you'll provide me a gif of the end result of your modal animation I can add it to the README.md and to the credit section :)

[Nickersoft]

Hey @mmazzarolo, so I poked around the code and was able to make it so now the animationIn and animationOut can accept either a string (which uses an animation from the default registry) or an animatable registry object in the following form (as you would when registering it globally):

{
    from: {},
    to: {}
}

This approach leverages the createAnimation() and registerAnimation() methods of react-native-animatable, and seems to work for my purposes. I can open a PR, but first.. what are your thoughts on this? This way, people can export their animation definitions as objects and using them would be as simple as:

import {myAnimations} from './helpers';
// ...
<Modal animationIn={myAnimations.fadeIn}>

I considered the animatableDefinitions prop, but thought it might be pointless for users to pass in their entire animation registry if they are only using two of the animations. Plus, this way it doesn't require people to use a registry, if they aren't using react-native-animatable in their app already.

[Nickersoft]

Oh, and you can check out my fix here.

[mmazzarolo]

That's neat, I like this approach even more than the one I suggested.
Go with the PR 👌

[mmazzarolo]

One more think, could you also update the README too?
Otherwise I'll do it, no worries.

Adding the prop to the list and pointing somewhere to the react-native-animatable docs (on creating custom animation) could be enough.

[Nickersoft]

Sure thing. I'll get that done and post a link to the PR when I'm done. One question though... I noticed there is some type information in index.d.ts. It's obviously TypeScript (which I've never used) but to me looks a lot like Facebook Flow type checking (which I have used). Is the syntax the same? If so, I can go and update that too, as I don't want any type errors being thrown. I already updated the PropTypes definition.

[Nickersoft]

All set: #72

[mmazzarolo]

Yes, it's typescript typedefinition, but your change shouldn't break anything even without updating it.
Maybe @iRoachie ( 👼 ) can update it once the PR gets merged?

I also noticed one thing that can be improved in my opinion.
Since your declaring the animation info as a component internal field (this.animationOut = animationOut) you won't be able to update it at runtime.
I know that it might be something that doesn't interest you directly, but if you could handle this case too it would be awesome (I could give it a shot too otherwise).
The easiest way to to it in my opinion is to move the logic you put in the constructor in a class method (something like _buildAnimations = (props: Props => {}) and call this method both in the constructor (passing the props as a parameter) and in componentWillReceiveProps (passing nextProps as a parameter).
What do you think?

[Nickersoft]

Consider it done @mmazzarolo :) The PR is now updated.

[mmazzarolo]

Landed on v4.0.0, thanks a lot @Nickersoft.

Feel free to post here or send any PR regarding this issue and I'll reopen it asap.

@iRoachie, the only difference between this version and the previous one in the typedefinition is that the animationIn and animationOut props now can either be string or { from: Object, to: Object } (don't know how to declare it in TS)

[iRoachie]

@mmazzarolo Here ya go #73