Skip to content
This repository has been archived by the owner on Feb 8, 2020. It is now read-only.

feat: remove necessity of wrapping navigators into container #21

Closed
wants to merge 2 commits into from

Conversation

osdnk
Copy link
Member

@osdnk osdnk commented Jul 21, 2019

No description provided.

@codecov-io
Copy link

codecov-io commented Jul 21, 2019

Codecov Report

Merging #21 into master will increase coverage by 2.07%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/NavigationContainer.tsx 100% <100%> (ø) ⬆️
src/createNavigator.tsx 85.71% <80%> (+85.71%) ⬆️
src/useNavigationHelpers.tsx 100% <0%> (ø) ⬆️
src/SceneView.tsx 94.73% <0%> (ø) ⬆️
src/NavigationContext.tsx
src/useNavigation.tsx

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 51200ba...5bfaad8. Read the comment docs.

@brentvatne
Copy link
Member

the motivation for explicit containers was two-fold:

  1. people were rendering navigators in nested components without realizing the implications of doing this - it would basically create a new navigation container with a navigation subtree that was disconnected from the parent, leading to some confusing behavior for people. in the new design on this repo this wouldn't be an issue because rendering navigators this way is one of the main motivations behind the design.
  2. on web/native we want to use different navigation containers: https://github.com/react-navigation/web/blob/master/src/createBrowserApp.js and https://github.com/react-navigation/native/blob/master/src/createAppContainer.js. so, if we provide a default container then we either have to bias towards one of them or include both and pick automatically depending on platform.

@osdnk
Copy link
Member Author

osdnk commented Jul 21, 2019

  1. It’s not an issue
  2. Container is so far very small and designed for mobile. Assuming that most of react navigation users are focused on mobile and a lot of them does not need deeplinking etc, can’t we assume it will cover most of cases?

@satya164
Copy link
Member

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.

@osdnk
Copy link
Member Author

osdnk commented Jul 22, 2019

@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', () => {
Copy link
Member

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);
Copy link
Member

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';
Copy link
Member

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

Copy link
Member

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) {
Copy link
Member

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

@osdnk
Copy link
Member Author

osdnk commented Jul 27, 2019

drob it

@osdnk osdnk closed this Jul 27, 2019
@satya164 satya164 deleted the @osdnk/no-need-to-wrap-into-container branch July 27, 2019 13:54
satya164 pushed a commit that referenced this pull request Aug 18, 2019
* Hook up tabBarOnPress

* Move onTabPress logic to createTabNavigator

* Use old logic for determining focus state

* Use navigation.isFocused()

* Reorder jumpTo/onTabPress

* [email protected]
satya164 pushed a commit that referenced this pull request Aug 18, 2019
* Hook up tabBarOnPress

* Move onTabPress logic to createTabNavigator

* Use old logic for determining focus state

* Use navigation.isFocused()

* Reorder jumpTo/onTabPress

* [email protected]
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants