Skip to content

[TM] Add spec for AnimatedModule#24911

Closed
thymikee wants to merge 4 commits into
facebook:masterfrom
thymikee:feat/type-animated-module
Closed

[TM] Add spec for AnimatedModule#24911
thymikee wants to merge 4 commits into
facebook:masterfrom
thymikee:feat/type-animated-module

Conversation

@thymikee
Copy link
Copy Markdown
Contributor

Summary

Part of #24875. Added strict-local to the NativeModuleHelper btw.

Changelog

[General] [Added] - TM add spec for AnimatedModule

Test Plan

Flow green.

@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. p: Callstack Partner: Callstack Partner labels May 16, 2019
@thymikee thymikee force-pushed the feat/type-animated-module branch from 98799ad to 20650b3 Compare May 16, 2019 17:34
@react-native-bot react-native-bot added API: Animated Type: Enhancement A new feature or enhancement of an existing feature. labels May 16, 2019
@uqmessias
Copy link
Copy Markdown
Contributor

I was working on that, but I'm glad to see it's finished \o/

@thymikee
Copy link
Copy Markdown
Contributor Author

@uqmessias so it seems like you're perfectly eligible to help reviewing this 😊

@ericlewis ericlewis added the Flow label May 16, 2019
Comment thread Libraries/Animated/src/NativeAnimatedModule.js Outdated
Comment thread Libraries/Animated/src/NativeAnimatedHelper.js Outdated
Copy link
Copy Markdown
Contributor

@fkgozali fkgozali left a comment

Choose a reason for hiding this comment

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

otherwise looks good!

|};

export type AnimatedNodeConfig = {|
type?:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should this be required? (no ?)

|};

export type AnimatingNodeConfig = {|
type?: 'frames' | 'spring' | 'decay',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this too should be required?

animationId: ?number,
nodeTag: ?number,
config: AnimatingNodeConfig,
endCallback: EndCallback,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

type imported from other files doesn't work well with our codegen atm, so just inline the type here or copy the type over in this file

@thymikee
Copy link
Copy Markdown
Contributor Author

@fkgozali addressed the feedback, thanks!


export type AnimatedNodeConfig = {|
type:
| 'style'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm actually we don't yet support enums, so I'll just update this to string when I import to FB, for now... Sorry for the confusion. cc @RSNara

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@react-native-bot
Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @thymikee in 116ac65.

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label May 23, 2019
M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
Summary:
Part of facebook#24875. Added `strict-local` to the NativeModuleHelper btw.

## Changelog

[General] [Added] - TM add spec for AnimatedModule
Pull Request resolved: facebook#24911

Reviewed By: rickhanlonii

Differential Revision: D15434114

Pulled By: fkgozali

fbshipit-source-id: ea9215306ebf969795ce755270b8fdc14c52da9c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API: Animated CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Flow Merged This PR has been merged. Native Module p: Callstack Partner: Callstack Partner Type: Enhancement A new feature or enhancement of an existing feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants