This repository has been archived by the owner on May 8, 2019. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tab navigator experiments #19
base: master
Are you sure you want to change the base?
Tab navigator experiments #19
Changes from all commits
82870c8
8b4b8f2
aaf0f84
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Does "screen" has to be a function?
I would unify it with StackNavigator and just make it a React element. It will make the API much easier. If we need a functional form, we can always wrap it later on our side.
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.
We don't need users to define a string of a route explicitly. I know that TabNavigator's router requires us to use a concept of string to register a route.
We can do better.
There are two ways of handling that:
a) StackNavigator's way - we define only one "route", let's call it Container. Every "route" is a "" element that accepts current screen to render as a prop. For example, "" where "EditUser(5)" could be a ReasonML variant.
b) Define a hashing function that can generate a UUID based on a variant it is given. For example, for "EditUser(5)", this could be "Route_EditUser_5". It's representation is not important as long as the function is stable and can generate same string for a given variant. This will be only used internally by the React Navigation.
Given that TabNavigator doesn't have StackNavigator's limitations, I propose we go with "b" for the sake of simplicity. We can always change it later, transparently for the end users.
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.
Do we pass the Variant to the navigate function, or not?
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.
Yeah in the end we need to pass a variant. We don't want to use string. This is just an experiment. This is not working yet. navigation is WIP.
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.
Ok, would be great to add a comment there to know what's going there
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.
In this case we don't. "NavigationProp" module is a binding to React Navigation's "navigation" that operates on strings and is not aware of ReasonML variant.
This is going to be super similar to
bs-react-navigation/src/StackNavigator.re
Lines 26 to 40 in aaf0f84
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.
We should translate variant to string here: https://github.com/callstackincubator/bs-react-navigation/pull/19/files#diff-f5a576ae9729bbd89c5aa8c4a2bf67ffR83
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.
This should accept a "navigation" prop so that we can actually talk to a real "navigation". See how StackNavigation handles that. The code will be exactly the same, except for the "routeName" part. Depending on the approach you take, this will be either similar to StackNavigation (passing variant to a Container route) or generating a hash (to be used as a routeName) from a given variant.
Either way, "navigate" should accept a "tab" (variant) instead of a "routeName" (string). "NavigationProp.navigate" is good - ReactNavigation operates on "string"s and we should keep it that way.
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.
As already said, "tabName" could be generated dynamically. "screen" doesn't have to be a function, we can wrap it here if needed.
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.
makeNavigationProp
in the future will accept original navigation prop. How do we handle that case? I know that the status of navigation prop is in WIP but I'm wondering how we handle thisThere 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.
I wonder that too 😉 We need to discuss that. You, me and @grabbou
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.
I think we should handle that just like we handle it in StackNavigator ->
bs-react-navigation/src/StackNavigator.re
Lines 55 to 63 in aaf0f84