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

Add initialRouteKey for StackRouter #3540

Merged
merged 11 commits into from Mar 3, 2018

Conversation

Projects
None yet
5 participants
@nico1510
Contributor

nico1510 commented Feb 17, 2018

When initializing the state for a StackRouter the keys for the inital routes are generated instead of using the routeName like it is done in the TabRouter

return {
key: routeName,
routeName,
params,
};
This makes it impossible to navigate to an exisiting (the initial one) screen with the newly added key property without pushing a new screen.

nico1510 added some commits Feb 17, 2018

Merge pull request #1 from nico1510/StackRouter-initial-key-fix
use initialRouteName as key when initializing StackRouter
@nico1510

This comment has been minimized.

Contributor

nico1510 commented Feb 17, 2018

if this change makes any sense I will fix the tests. They fail currently because they always expect the generated keys.

@brentvatne

This comment has been minimized.

Member

brentvatne commented Feb 17, 2018

what happens if you use the stacknavigator in two places?

@nico1510

This comment has been minimized.

Contributor

nico1510 commented Feb 17, 2018

Hm I can't really think of a use case where that would make sense... Could you provide one maybe ?

@brentvatne

This comment has been minimized.

Member

brentvatne commented Feb 17, 2018

we currently do not limit people from using a stacknavigator in multiple places. if we were to land this pr we are essentially imposing a new restriction on people. the fact that tabnavigator apparently doesn't handle this is unfortunate. one example for how this might happen right now: you have a tabnavigator for admins and a tabnavigator for normal users, perhaps they share the same "notifications" tab, which points to a notifications stack.

@nico1510

This comment has been minimized.

Contributor

nico1510 commented Feb 17, 2018

Not sure if understand that correctly but...
Then these 2 Stackrouters should be probably mounted under a distinct routeName. Otherwise it would already not be clear on where to navigate. If they are mounted under a distinct routeName then the keys would also be different.

@brentvatne

This comment has been minimized.

Member

brentvatne commented Feb 22, 2018

@nico1510 - can we add initialRouteKey config option instead? we may deprecate it in the future but with this at least we have a non-breaking change and still solve the issue at hand

@nico1510

This comment has been minimized.

Contributor

nico1510 commented Feb 22, 2018

Yes sure I can add that :)

@react-navigation-ci

This comment has been minimized.

@react-navigation-ci

This comment has been minimized.

@nico1510

This comment has been minimized.

Contributor

nico1510 commented Feb 24, 2018

@brentvatne the functionality is now behind a config flag and I added a test case.

@react-navigation-ci

This comment has been minimized.

@codecov-io

This comment has been minimized.

codecov-io commented Feb 24, 2018

Codecov Report

Merging #3540 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3540      +/-   ##
==========================================
+ Coverage   69.95%   69.97%   +0.01%     
==========================================
  Files          54       54              
  Lines        1734     1735       +1     
==========================================
+ Hits         1213     1214       +1     
  Misses        521      521
Impacted Files Coverage Δ
src/routers/StackRouter.js 94.71% <100%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bb91a6...9257f42. Read the comment docs.

@brentvatne brentvatne changed the title from use initialRouteName as key when initializing StackRouter to Add initialRouteKey for StackRouter Feb 26, 2018

@@ -405,6 +405,7 @@ declare module 'react-navigation' {
initialRouteParams?: NavigationParams,
paths?: NavigationPathsConfig,
navigationOptions?: NavigationScreenConfig<*>,
initialRouteKey?: 'initialRouteName' | 'generated',

This comment has been minimized.

@brentvatne

brentvatne Feb 26, 2018

Member

this typedef doesn't seem correct. I think it's just initialRouteKey?: string

key: action.key || generateKey(),
key:
action.key ||
(initialRouteKey && initialRouteKey === 'initialRouteName'

This comment has been minimized.

@brentvatne

brentvatne Feb 26, 2018

Member

we shouldn't special case this, if you want it to be the same as the initial route name then just pass in a key that is the same as the initial route name

@nico1510

This comment has been minimized.

Contributor

nico1510 commented Feb 26, 2018

@brentvatne ah now I see what you meant. I updated the PR

@react-navigation-ci

This comment has been minimized.

@brentvatne brentvatne merged commit c6301ab into react-navigation:master Mar 3, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@nico1510 nico1510 deleted the nico1510:StackRouter-initial-key-fix branch Mar 3, 2018

brentvatne added a commit that referenced this pull request Mar 8, 2018

Add initialRouteKey for StackRouter (#3540)
* use initialRouteName as key when initializing StackRouter

* fix null headerLeft

* merge back

* fixed tests

* use config flag

* fixed snapshots

* implemented requested changes
@BrandonSmith

This comment has been minimized.

Contributor

BrandonSmith commented Mar 8, 2018

@nico1510 @brentvatne With the addition of initialRouteKey, how is it intended to be used? Seems StackNavigator fails to pass it down to StackRouter.

@nico1510

This comment has been minimized.

Contributor

nico1510 commented Mar 9, 2018

Oops yes you're right it's not passed. Should be an easy fix though. I can create a PR for this or you can create one yourself if you find the time 😁

@BrandonSmith

This comment has been minimized.

Contributor

BrandonSmith commented Mar 9, 2018

No problem. I've got a local StackNagivator with the fix. I'll get a PR with the needed change.

BrandonSmith added a commit to BrandonSmith/react-navigation that referenced this pull request Mar 9, 2018

BrandonSmith added a commit to BrandonSmith/react-navigation that referenced this pull request Mar 9, 2018

brentvatne added a commit that referenced this pull request Mar 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment