feat: accept router to be a function #22
Conversation
Codecov Report
@@ Coverage Diff @@
## master #22 +/- ##
==========================================
+ Coverage 89.51% 92.21% +2.69%
==========================================
Files 18 16 -2
Lines 248 244 -4
Branches 58 58
==========================================
+ Hits 222 225 +3
+ Misses 24 17 -7
Partials 2 2
Continue to review full report at Codecov.
|
satya164
force-pushed
the
@satya164/set-options
branch
from
61dbbd7
to
8a09d94
Compare
|
Can you drop the commit regarding |
satya164
force-pushed
the
@satya164/set-options
branch
2 times, most recently
from
109e1f1
to
0519693
Compare
|
@satya164 My changes are built on thew top of yours. If you're force-pushed commit with another hash, it might imply problems. I can do it after merging your PR. |
satya164
force-pushed
the
@satya164/set-options
branch
2 times, most recently
from
6793b09
to
83477e3
Compare
satya164
force-pushed
the
@osdnk/allow-router-to-be-a-function
branch
from
e980e36
to
56fb0fa
Compare
src/useNavigationBuilder.tsx
Outdated
| Options extends DefualtOptions = DefualtOptions | ||
| >(router: RouterHelper<any, Options>, options: Options) { | ||
| const { current: currentRouter } = React.useRef<Router<any>>( | ||
| typeof router === 'function' ? router(options) : router |
There was a problem hiding this comment.
I think it's better to just always make it a function rather than accepting both object and function
There was a problem hiding this comment.
I think we shouldn't pass things like children to the router. It'll be problematic if someone writing a custom router uses children.
src/useNavigationBuilder.tsx
Outdated
| @@ -79,7 +84,7 @@ export default function useNavigationBuilder<ScreenOptions extends object>( | |||
| ); | |||
|
|
|||
| const [initialState] = React.useState(() => | |||
| router.getInitialState({ | |||
| currentRouter.getInitialState({ | |||
| routeNames, | |||
| initialRouteName, | |||
There was a problem hiding this comment.
Since we now pass initialRouteName as an option, we could remove it from here
src/useNavigationBuilder.tsx
Outdated
| @@ -79,7 +84,7 @@ export default function useNavigationBuilder<ScreenOptions extends object>( | |||
| ); | |||
|
|
|||
| const [initialState] = React.useState(() => | |||
| router.getInitialState({ | |||
| currentRouter.getInitialState({ | |||
| routeNames, | |||
| initialRouteName, | |||
| initialParamsList, | |||
There was a problem hiding this comment.
Let's rename this to routeParamList, since it's not really initial as it changes when children change.
src/useNavigationBuilder.tsx
Outdated
| ScreenOptions extends object, | ||
| Options extends DefualtOptions = DefualtOptions | ||
| >(router: RouterHelper<any, Options>, options: Options) { | ||
| const { current: currentRouter } = React.useRef<Router<any>>( |
There was a problem hiding this comment.
Since there's always going to be one router, I think no need to say currentRouter, router looks nicer 😛
src/useNavigationBuilder.tsx
Outdated
| ) { | ||
| export default function useNavigationBuilder< | ||
| ScreenOptions extends object, | ||
| Options extends DefualtOptions = DefualtOptions |
src/types.tsx
Outdated
| @@ -55,6 +56,16 @@ export type ActionCreators<Action extends NavigationAction> = { | |||
| [key: string]: (...args: any) => Action; | |||
| }; | |||
|
|
|||
| export type DefualtOptions = { | |||
osdnk
force-pushed
the
@osdnk/allow-router-to-be-a-function
branch
from
e3d24a8
to
e81116a
Compare
satya164
force-pushed
the
@osdnk/allow-router-to-be-a-function
branch
from
69eceed
to
fec3e7f
Compare
No description provided.