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.

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

Refactor Navigation (see #51) #89

Closed

Conversation

fabriziomoscon
Copy link
Contributor

@fabriziomoscon fabriziomoscon commented Jul 22, 2016

Inspired by:
https://github.com/facebook/react-native/blob/master/Examples/UIExplorer/js/NavigationExperimental/NavigationCardStack-NavigationHeader-Tabs-example.js
and other example online

tested on Android on physical device
not tested on ios because of #77

@fabriziomoscon fabriziomoscon force-pushed the refactor-nav-exp branch 2 times, most recently from 8bb15c6 to 64da2b1 Compare July 22, 2016 15:00
const currentTab = navigationState.getIn(['routes', navigationState.get('index')]);
const tabs = navigationState.get('tabs');
const tabKey = tabs.getIn(['routes', tabs.get('index')]).get('key');
const currentTab = navigationState.get(tabKey);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the way to find currentTab accordingly to the navigationState changes

- use NavigationExperimental Card Stack
- simplify navigationState separating tabs reducer from scene reducer
- simplify navigation views
- handle Android back button
- remove unnecessary import of AppRouter - fixes futurice#85
@@ -50,7 +49,7 @@ const AppView = React.createClass({

return (
<View style={{flex: 1}}>
<NavigationViewContainer router={AppRouter} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixes #85

@wuzhuzhu
Copy link

wuzhuzhu commented Aug 3, 2016

+1 Approve cool, want to use this one.

@jevakallio
Copy link
Contributor

@fabriziomoscon just got through a first pass review on this, awesome job! 💯 🍻 This is a lot cleaner and more future-proof that our previous solution!

There are a couple of tiny fixes I'll need to make to merge this:

  • When mergin your branch to master, the TabBar tests fail. To fix this, we need to upgrade react-native-mock to 0.2.5, because the older version does not provide the NavigationExperimental propTypes, which are now used in validation.
  • The ColorView doesn't pass a title to the subsequent ColorViews, causing a propType validation error on the NavigationHeaderTitle.

I want to get this merged ASAP, so I'll just cherry pick your changes to a new branch, fix and test these, and push it up.

@fabriziomoscon
Copy link
Contributor Author

thanks. If you prefer I can fix those 2 point and repush

@jevakallio
Copy link
Contributor

I just merged this to master 🎉 Thanks @fabriziomoscon, hit me up sometime and I'll buy you a beer!

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.

Why do both modules/AppView and modules/navigation/NavigationView import AppRouter?
3 participants