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.

Sign up for GitHub

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

Jump to bottom

[CLI Templates] Included NativeAnimation module on iOS in the starter project #10783

Closed
wants to merge 1 commit into from
Closed

[CLI Templates] Included NativeAnimation module on iOS in the starter project #10783

wants to merge 1 commit into from

Conversation

sreejithr
Copy link
Contributor

Fixes #10638.

Added NativeAnimation library to the starter project iOS generator. Passing useNativeDriver: true to Animated config will enable the app to tap into the native code for animations.

Test plan

Init a RN project and animate an element. Enable native driver as follows:

Animated.timing(
      this.state.value,
      {
        toValue: 300,  // some value
        useNativeDriver: true
      }
    ).start();

Earlier, this used to crash.

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @jhen0409 and @callmephilip to be potential reviewers.

@sreejithr sreejithr mentioned this pull request Nov 7, 2016
Closed
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 7, 2016
@hramos
Copy link
Contributor

hramos commented Nov 7, 2016

What's the motivation behind this? Does this only prevent crashes for apps that use native Animated?

@janicduplessis
Copy link
Contributor

@hramos Yes, we shipped native animations for touchables and soon NavigationExperimental so we should include it by default now. It is also already included on android.

hramos
hramos reviewed Nov 9, 2016
View reviewed changes
local-cli/generator-ios/templates/xcodeproj/project.pbxproj
remoteRef = 5E9157321DD0AC6500FF2AA8 /* PBXContainerItemProxy */;
sourceTree = BUILT_PRODUCTS_DIR;
};
5E9157351DD0AC6500FF2AA8 /* libRCTAnimation-tvOS.a */ = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional (tvOS)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was not intentional. If you try linking libRCTAnimation.a in "Build Phases" and check your project's diff, you'll see that Xcode has done this automatically. Should I remove this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't done any Apple TV development yet, and I am not familiar with how Xcode handles static libraries for projects that may target tvOS. @dlowder-salesforce perhaps you can chime in?

Copy link
Contributor

@douglowder douglowder Nov 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

All libraries should have two targets now, one for iOS and one for tvOS. So this is probably correct. If you have integration test automation for the new library, and it is passing on tvOS, then the project is ok :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@hramos I just noticed that this is for generating starter iOS projects. From what I'm seeing, there are tvOS targets for the other libraries included, so this should be fine. At some point fairly soon, I'll get to supporting tvOS fully for starter projects.

@janicduplessis
Copy link
Contributor

LGTM, I'll let @hramos ship it.

@sreejithr
Copy link
Contributor Author

@janicduplessis Should I squash my commits or something? This is my first contribution to React Native :)

@janicduplessis
Copy link
Contributor

Not needed. the bot will take care of it :)

@hramos
Copy link
Contributor

hramos commented Nov 10, 2016

Great, thanks for your help reviewing this PR, @janicduplessis and @dlowder-salesforce. I can't import it just yet - we're holding off on updating app templates pending an improvement to how react-native upgrade works - but you may consider this PR approved nonetheless. I'll circle back and land it once that related work is finished.

@hramos hramos self-assigned this Nov 10, 2016
@hramos hramos changed the title Included NativeAnimation module on iOS in the starter project [CLI Templates] Included NativeAnimation module on iOS in the starter project Nov 10, 2016
@mkonicek
Copy link
Contributor

mkonicek commented Nov 10, 2016 •
edited
Loading

Yes this will likely cause conflicts when people run react-native upgrade and had local changes in the Xcode project file, e.g. when using 3rd-party native modules. People might have to overwrite the project file and run react-native link again to fix it :(

The new planned react-native upgrade should help by doing a 3-way merge but it will take a while to ship and the enabling native Animated is pretty important. Therefore I think we should ship it even before the new upgrade command is ready.

@facebook-github-bot shipit

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Nov 10, 2016
@facebook-github-bot
Copy link
Contributor

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

@adrienbataille adrienbataille mentioned this pull request Nov 29, 2016
Closed
DanielMSchmidt pushed a commit to DanielMSchmidt/react-native that referenced this pull request Jan 4, 2017
@sreejithr @DanielMSchmidt
Included NativeAnimation module on iOS in the starter project
84f8917 
Summary:
Fixes facebook#10638.

Added NativeAnimation library to the starter project iOS generator. Passing `useNativeDriver: true` to `Animated` config will enable the app to tap into the native code for animations.

**Test plan**

Init a RN project and animate an element. Enable native driver as follows:

```
Animated.timing(
      this.state.value,
      {
        toValue: 300,  // some value
        useNativeDriver: true
      }
    ).start();
```
Earlier, this used to crash.
Closes facebook#10783

Differential Revision: D4159386

Pulled By: mkonicek

fbshipit-source-id: 993481a31b4446eab24ef4ee35ae1941d7a7aae9
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
@sreejithr
Included NativeAnimation module on iOS in the starter project
4406fa5 
Summary:
Fixes #10638.

Added NativeAnimation library to the starter project iOS generator. Passing `useNativeDriver: true` to `Animated` config will enable the app to tap into the native code for animations.

**Test plan**

Init a RN project and animate an element. Enable native driver as follows:

```
Animated.timing(
      this.state.value,
      {
        toValue: 300,  // some value
        useNativeDriver: true
      }
    ).start();
```
Earlier, this used to crash.
Closes facebook/react-native#10783

Differential Revision: D4159386

Pulled By: mkonicek

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

@douglowder douglowder douglowder left review comments

@hramos hramos hramos left review comments

Assignees

@hramos hramos

Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sreejithr @facebook-github-bot @hramos @janicduplessis @mkonicek @douglowder