search

futurice / pepperoni-app-kit

Pepperoni - React Native App Starter Kit for Android and iOS
http://getpepperoni.com
MIT License
4.63k stars 645 forks source link

React Navigation (rebased PR) #189

Closed FruitieX closed 7 years ago

FruitieX commented 7 years ago

https://reactnavigation.org/ :rocket:

FruitieX commented 7 years ago

TODO: Tab bar icons on iOS done

Here's how we can do that

Other than that, tested on both Android & iOS

krivachy commented 7 years ago

Code looks good I think there are 3 changes that need to be made before merging:

1) JS tests are failing. 2) Icons for tab bar :) 3) Behaviour of the app/navigator has changed and the look and feel of the app has changed between android/iOS. The stack navigator now overlays the tabbar, previously it was in the same tab. Personally I think the use-case when the screens stack inside the tab makes more sense (just look at Twitter/instagram/etc.) Also if we're going for Android Material UI it might make sense to go full on.

Screenshot 1

Screenshot 2

Screenshot 3

FruitieX commented 7 years ago

Thanks for the review.

  1. The testing issue might be solved by a RN version upgrade, but in the mean time there's a workaround which I will commit.
  2. Will do :)
  3. I think I first had the navigators the other way around, which resulted in the AppBar being below the Tab bar which looked a little funny. :) But let's see if it's possible to remedy somehow.

About material-ui, I have something that looks like this at the moment:

Android screenshot

FruitieX commented 7 years ago

OK, tests are fixed and iOS now looks like this:

Screenshot 1 Screenshot 2

krivachy commented 7 years ago

Header seems to jump on first render and when switching screens on iOS. This is with Debugging enabled, but the jump is still visible without debugging enabled:

Screen gif 1 Screen gif 2

FruitieX commented 7 years ago

I've been unable to reproduce the header title jumping around in the iOS simulator.

I'd say it's an issue with react-navigation. There was a somewhat related issue recently closed, maybe the problem is fixed now.

sercanov commented 7 years ago

Hey @FruitieX , any ETA on this ? :)

FruitieX commented 7 years ago

@sercanov I'll fix the navigation reducer today and then hopefully this PR can be merged

FruitieX commented 7 years ago

Awaiting review, then we are good to go :+1:

sercanov commented 7 years ago

I'll definitely will try and give feedback asap. I'm sick of NavigationExperimental enough..

Actually I integrated react-navigation into pepperoni in my local, its more stable than NE but it still has lot to go, i think they'll release stable v1 in april so we should follow here to update accordingly.

Cheers and thanks!

tino-junge commented 7 years ago

Reviewed once more and as discussed we keep the navigation and icon behavior for android for now and change it if need be. Good job @FruitieX !