Skip to content

Another way to define route properties?#6

Closed
pedro wants to merge 2 commits into
wix:masterfrom
pedro:resolve-routes
Closed

Another way to define route properties?#6
pedro wants to merge 2 commits into
wix:masterfrom
pedro:resolve-routes

Conversation

@pedro
Copy link
Copy Markdown

@pedro pedro commented Mar 25, 2016

Opening mostly to start a discussion:

First of all thanks so much for this and react-native-controllers! I tried pretty much all navigator permutations out there, and this is the only one that gets the job done without issues. Big +999 for the idea of delegating navigation/navbar to obj-c, I am also unsure why there's so much effort into trying to reproduce all of this in JS.

Now, the one thing I miss from Navigator is the ability to lazily configure a route.

For instance, here to set the screen title you pretty much have to do while pushing/opening a modal with it – whereas with Navigator you have the opportunity to set that up both from renderScene or from the navigationBar.

In this pull I'm adding an optional callback passed before the screen params are resolved, so you can define your titles elsewhere, like:

// routes defined somewhere:
Routes = {
  login: {
    title: 'Please Login'
  }
}

// navigation setup
Navigation.startSingleScreenApp({
  screen: {
    screen: 'main',
  },
  routeResolver: (original) => {
    let route = Routes[original.screen];
    return React.addons.update(route, { $merge: original });
  }
});

I suspect this could be helpful to others migrating from Navigator.

BUT, going a bit further, I do love how we can define navigator styles from within the component via a static property! Would be awesome to be able to define the screen title right there, along with the actual component, its buttons and navigator style.

So in summary, two proposals:

  • Take optional callback to add a layer of indirection between the route params and what's passed on to the screen (this pull)
  • Take more generic screen options via a static property. Not just navigatorStyle, but pretty much everything you can pass to push and others. They would just like the navigator style: serve as default, but can still be overrided by route params.

Hope this makes sense, if so I can obviously open a pull for the 2nd above too.

pedro added 2 commits March 24, 2016 14:53
so you can further customize routes without changing every caller.
can be used for default navigator styles/etc
@smooJitter
Copy link
Copy Markdown

What's the point? What value does it add?

@pedro
Copy link
Copy Markdown
Author

pedro commented Mar 25, 2016

Basically it allows you to avoid code repetition.

Lets say you have two buttons taking users to the login screen; currently you have to set the title in there both times:

this.props.navigator.push({ screen: 'login', title: 'Login' });

So if you ever want to change the title of that component you have to go chase who's pushing it. I'm proposing that we allow users to set title and other attributes somewhere else, so they only have to define it once (although components can still override when pushing).

@smooJitter
Copy link
Copy Markdown

Now that I'm further along, I do understand the utility of this.

@DanielZlotin
Copy link
Copy Markdown
Contributor

DanielZlotin commented Jul 14, 2016

@pedro What you're probably looking for is the ability for each screen to be able to define its own options (for example static options = {title: 'Login'} in the screen component etc.).. see https://github.com/wix/react-native-navigation/issues/68.
This is in the roadmap for v2.0

@drorbiran drorbiran added this to the 2.0.0 milestone Aug 3, 2016
@drorbiran drorbiran removed the v2.0.0 label Aug 3, 2016
steverob pushed a commit to spritlesoftware/react-native-navigation that referenced this pull request Jun 21, 2018
Merge wix/master into ours
shalehaha pushed a commit to shalehaha/react-native-navigation that referenced this pull request Dec 15, 2018
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