Skip to content

Use Exponent: Look ma, no native modules 👌 #12

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

Closed
wants to merge 8 commits into from

Conversation

cem2ran
Copy link

@cem2ran cem2ran commented Mar 6, 2017

Converted the app to use Exponent as all the needed modules already were provided by Exponent. (almost, not Microsoft code-push, but Exponent has their own flavour built-in)

Some tests are failing due to SyntaxError: Unexpected token import. Not sure why. I believe the snapshots just have to be updated.

@@ -15,7 +15,7 @@ import moment from 'moment';

import type { ScheduleTalk } from '../../types';

import Splash from 'react-native-smart-splash-screen';
// import Splash from 'react-native-smart-splash-screen';
Copy link
Author

Choose a reason for hiding this comment

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

Alright, I cheated slightly. But maybe it can do without?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually this splash screen is easy to make in Exponent! see PR to your repo

Copy link
Contributor

@thekevinbrown thekevinbrown Mar 6, 2017

Choose a reason for hiding this comment

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

That component is there just to make sure there's a smooth transition from the app loading to the animation at start. The flow currently is:

  • LaunchScreen.storyboard shows the image.
  • react-native-smart-splash-screen shows the same image while react native populates the Schedule scene.
  • Once the component is mounted, react-native-smart-splash-screen fades out, revealing the 'real' components, which then animate.

I'm not sure in general whether we want to move to Exponent or not, but that's what it's currently doing.

Copy link
Member

Choose a reason for hiding this comment

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

@brentvatne "see PR to your repo" which one?

Copy link
Contributor

Choose a reason for hiding this comment

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

@JedWatson - it was to @cem2ran's - cem2ran#1

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we don't have great support for a splashscreen that animates into the app for various reasons. It works meh on iOS right now, didn't bother tweaking it further because the image loader will always potentially cause a flicker. Either way, I'll keep the fork updated at https://expo.io/@community/reactconf2017 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@JedWatson - might be worth pulling my changes for useNativeDriver in the schedule ListView in though

Copy link
Contributor

Choose a reason for hiding this comment

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

@brentvatne, thanks for that! I'll pull in some of those changes, but for example I don't see a need for an Animated.ListView as the list view never moves. Let me know if I'm missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

@blargity - AnimatedListView lets you run the animations in response to onScroll on the UI thread: http://facebook.github.io/react-native/blog/2017/02/14/using-native-driver-for-animated.html -- it makes the animation happen immediately in response to the scroll and so it doesn't drop frames when you page in new rows in the listview

Copy link
Contributor

Choose a reason for hiding this comment

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

@brentvatne, thanks for that. I've tried to pull those changes in but I'm still getting an error that I've never seen before. Details are in #44, and the work is on a branch to make it easy to reproduce.

@thekevinbrown
Copy link
Contributor

Thank you, but at this stage we're not looking to move over to Exponent. I'll go ahead and close this PR, but feel free to keep it up to date with master on your side!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants