feat: add native integration skeleton #48

osdnk · 2019-08-08T00:39:31Z

No description provided.

satya164 · 2019-08-08T01:03:20Z

packages/example/src/index.tsx

@@ -41,6 +46,8 @@ const PERSISTENCE_KEY = 'NAVIGATION_STATE';
 Asset.loadAsync(StackAssets);

 export default function App() {
+  const containerRef = React.useRef<NavigationContainerRef>(React.createRef());


You don't need to pass createRef into useRef, we can pass null and use the returned ref instead of .current

satya164 · 2019-08-08T01:04:50Z

packages/core/src/NavigationContainer.tsx

@@ -162,7 +167,7 @@ const Container = React.forwardRef(function NavigationContainer(
        "Any 'setState' calls need to be done inside 'performTransaction'"
      );
    }
-
+    wasStateModified.current = true;


shouldn't we check if state actually changed?

satya164 · 2019-08-08T01:06:11Z

packages/example/package.json

@@ -17,6 +17,7 @@
    "eject": "expo eject"
  },
  "dependencies": {
+    "@navigation-ex/native": "^0.0.1",


I think this will create a symlink, right? if just add it to the eslint config in root

satya164 · 2019-08-08T01:08:07Z

packages/native/src/useBackButton.tsx

+      if (castedRef.current !== null) {
+        castedRef.current
+          .dispatch({ type: 'GO_BACK' })
+          .then((wasHandled: boolean) => {


Use async/await maybe :)

satya164 · 2019-08-08T01:33:21Z

I have another thought. This wouldn't be a problem for us if we had a some kind of navigation.canGoBack method to query if we can go back in history. Each router can implement this method.

When you call this method, it'll recursively call parent canGoBack like the isFocused method does, and in the end, we have concrete info on if back button can be handled.

Similar to how the root container's dispatch dispatches the action to focused navigator, root container's canGoBack will call the focused navigator's canGoBack.

Now we can check this method in our container and only dispatch a back action if we can go back. No messing with how state updates, no returning promise from dispatch etc. canGoBack is also a useful method for users.

…gation-listeners

fixed

codecov-io · 2019-08-13T20:09:13Z

Codecov Report

Merging #48 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
+ Coverage    96.3%   96.31%   +<.01%     
==========================================
  Files          24       24              
  Lines         406      407       +1     
  Branches       87       87              
==========================================
+ Hits          391      392       +1     
  Misses         14       14              
  Partials        1        1

Impacted Files	Coverage Δ
packages/core/src/useNavigationBuilder.tsx	`100% <ø> (ø)`	⬆️
packages/core/src/useDescriptors.tsx	`100% <ø> (ø)`	⬆️
packages/core/src/NavigationContainer.tsx	`100% <100%> (ø)`	⬆️
packages/core/src/useNavigationHelpers.tsx	`100% <100%> (ø)`	⬆️

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 ed9954b...c13de83. Read the comment docs.

satya164

It's a bit hard to review with both changes in this PR. Can you rebase your PR and squash the commits so that there is a single commit for changes made only in this PR which I can easily review?

Or let's get the other PR merged first and rebase.

packages/example/src/index.tsx

satya164 · 2019-08-14T00:24:06Z

packages/native/src/index.tsx

+
+export { useBackButton };
+
+export default function useNativeIntegration(ref: NavigationContainerRef) {


move the hook to separate file since we only re-export in index files everywhere

satya164 · 2019-08-14T00:26:34Z

packages/core/src/NavigationContainer.tsx


  React.useImperativeHandle(ref, () => ({
+    performOnFocusedNavigator: (


Don't think we want to expose performOnFocusedNavigator. The exposed ref should look like every other navigation object. We should expose canGoBack method instead where we internally perform this on focused navigator and return the result.

osdnk added 2 commits August 8, 2019 01:38

feat: add native integration skeleton

1cab8b2

fix: remove custom emitting

ead6f4d

satya164 previously requested changes Aug 8, 2019

View reviewed changes

satya164 reviewed Aug 8, 2019

View reviewed changes

osdnk added 10 commits August 11, 2019 23:17

Merge remote-tracking branch 'remotes/origin/master' into @osdnk/native

cfd1ab0

refactor: simplify dispatching logic

f3f65c7

Merge branch '@osdnk/navigation-listeners' into @osdnk/native

f0b3553

refactor: make it reliable!

a486b98

fix: add dispatching action

9620d95

Merge remote-tracking branch 'remotes/origin/master' into @osdnk/navi…

e7c2b2c

…gation-listeners

fix: check file

51b2f8c

Merge branch '@osdnk/navigation-listeners' into @osdnk/native

90b8420

fix: types to the end

2bf94d2

fix: symlink

4dd383f

satya164 suggested changes Aug 14, 2019

View reviewed changes

satya164 force-pushed the @osdnk/native branch 4 times, most recently from 0883d97 to e202be4 Compare August 14, 2019 14:03

Merge branch 'master' into @osdnk/native

a971f37

satya164 force-pushed the @osdnk/native branch from e202be4 to a971f37 Compare August 14, 2019 14:03

satya164 merged commit 3fa5fe2 into master Aug 14, 2019

satya164 deleted the @osdnk/native branch August 14, 2019 14:04

satya164 pushed a commit that referenced this pull request Aug 18, 2019

feat: add native container with back button integration (#48)

b7735af

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add native integration skeleton #48

feat: add native integration skeleton #48

osdnk commented Aug 8, 2019

satya164 Aug 8, 2019 •

edited

Loading

satya164 Aug 8, 2019

satya164 Aug 8, 2019

satya164 Aug 8, 2019

satya164 commented Aug 8, 2019

codecov-io commented Aug 13, 2019 •

edited

Loading

satya164 left a comment

satya164 Aug 14, 2019

satya164 Aug 14, 2019


		export { useBackButton };

		export default function useNativeIntegration(ref: NavigationContainerRef) {


		React.useImperativeHandle(ref, () => ({
		performOnFocusedNavigator: (

feat: add native integration skeleton #48

feat: add native integration skeleton #48

Conversation

osdnk commented Aug 8, 2019

satya164 Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

satya164 Aug 8, 2019

Choose a reason for hiding this comment

satya164 Aug 8, 2019

Choose a reason for hiding this comment

satya164 Aug 8, 2019

Choose a reason for hiding this comment

satya164 commented Aug 8, 2019

codecov-io commented Aug 13, 2019 • edited Loading

Codecov Report

satya164 left a comment

Choose a reason for hiding this comment

satya164 Aug 14, 2019

Choose a reason for hiding this comment

satya164 Aug 14, 2019

Choose a reason for hiding this comment

satya164 Aug 8, 2019 •

edited

Loading

codecov-io commented Aug 13, 2019 •

edited

Loading