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

feat: add native integration skeleton#48

Merged
satya164 merged 13 commits intomasterfrom
@osdnk/native
Aug 14, 2019
Merged

feat: add native integration skeleton#48
satya164 merged 13 commits intomasterfrom
@osdnk/native

Conversation

@osdnk
Copy link
Copy Markdown
Member

@osdnk osdnk commented Aug 8, 2019

No description provided.

Comment thread packages/example/src/index.tsx Outdated
Asset.loadAsync(StackAssets);

export default function App() {
const containerRef = React.useRef<NavigationContainerRef>(React.createRef());
Copy link
Copy Markdown
Member

@satya164 satya164 Aug 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

);
}

wasStateModified.current = true;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we check if state actually changed?

Comment thread packages/example/package.json Outdated
"eject": "expo eject"
},
"dependencies": {
"@navigation-ex/native": "^0.0.1",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread packages/native/src/useBackButton.tsx Outdated
if (castedRef.current !== null) {
castedRef.current
.dispatch({ type: 'GO_BACK' })
.then((wasHandled: boolean) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use async/await maybe :)

@satya164
Copy link
Copy Markdown
Member

satya164 commented Aug 8, 2019

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.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Aug 13, 2019

Codecov Report

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

Impacted file tree graph

@@            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.

Copy link
Copy Markdown
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/example/src/index.tsx
Comment thread packages/native/src/index.tsx Outdated

export { useBackButton };

export default function useNativeIntegration(ref: NavigationContainerRef) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

listeners[0](navigation => navigation.dispatch(action));

React.useImperativeHandle(ref, () => ({
performOnFocusedNavigator: (
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@satya164 satya164 force-pushed the @osdnk/native branch 4 times, most recently from 0883d97 to e202be4 Compare August 14, 2019 14:03
@satya164 satya164 merged commit 3fa5fe2 into master Aug 14, 2019
@satya164 satya164 deleted the @osdnk/native branch August 14, 2019 14:04
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.

3 participants