-
Notifications
You must be signed in to change notification settings - Fork 41
feat: remove necessity of wrapping navigators into container #21
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21 +/- ##
==========================================
+ Coverage 89.21% 91.28% +2.07%
==========================================
Files 18 16 -2
Lines 241 241
Branches 56 58 +2
==========================================
+ Hits 215 220 +5
+ Misses 24 19 -5
Partials 2 2
Continue to review full report at Codecov.
|
the motivation for explicit containers was two-fold:
|
|
The container also handles hardware back button on mobile which I think all apps which support Android need to handle. And for webs, I think history integration is something people will want to have. So users would need to wrap their code in container still. Though we can still wrap by default with the base container without any additional functionality by default. When users use another container, that'll work fine. The only problem is that by not making it explicit, users may not realize that they need to do it to handle back button etc. I don't have any strong opinion on this. So lemme know what you prefer. |
@brentvatne it's up to you |
@@ -42,8 +45,12 @@ it('throws when setState is accessed without a container', () => { | |||
); | |||
}); | |||
|
|||
it('throws when performTransaction is accessed without a container', () => { | |||
expect.assertions(1); | |||
it('throws when performTransaction is accessed without a container but inside navigator', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't throw?
<TestNavigator.Navigator> | ||
<TestNavigator.Screen name="name" component={Test} /> | ||
</TestNavigator.Navigator>); | ||
render(element).update(element); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: new line before xd
@@ -1,6 +1,8 @@ | |||
import * as React from 'react'; | |||
import { ParamListBase, RouteConfig, TypedNavigator } from './types'; | |||
import Screen from './Screen'; | |||
import { NavigationStateContext } from './NavigationContainer'; | |||
import { NavigationContainer } from './index'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the import to NavigationContainer
file like above to avoid circular dep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: unrelated, but can you move the types import to bottom xd
|
||
// @ts-ignore | ||
const Navigator = <RawNavigator {...props} /> | ||
if (getState() === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: new line before xd
drob it |
* Hook up tabBarOnPress * Move onTabPress logic to createTabNavigator * Use old logic for determining focus state * Use navigation.isFocused() * Reorder jumpTo/onTabPress * [email protected]
* Hook up tabBarOnPress * Move onTabPress logic to createTabNavigator * Use old logic for determining focus state * Use navigation.isFocused() * Reorder jumpTo/onTabPress * [email protected]
No description provided.