Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing options props #35

Merged
merged 12 commits into from
Aug 11, 2020

Conversation

Freddy03h
Copy link
Member

Add and removing props and options regarding react-navigation v5 documentation.

Copy link
Member

@MoOx MoOx left a comment

Choose a reason for hiding this comment

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

Awesome! Are you confident with this improvements (= should we cut another release?)
Also could you specify the version of react-navigation you refered to somewhere?
Maybe update this https://github.com/reason-react-native/reason-react-navigation/blob/master/package.json#L5 & add other packages so we people know better the version?

@Freddy03h
Copy link
Member Author

I tried those changes on my own project this week-end, it work well :)

  • fix tabBar : function that return an element instead of being a component (my previous PR about it was a bit rushed ^^' )
  • add the view MaterialTopTabBar in MaterialTopTabs
  • add and remove many props and options on MaterialTopTabs, BottomTabs and Stack

For the peerDependencies I think we should add all the dependencies? But at least remove /core because it's now recommended to not add it (only add /native). Should I replace it with native only or should I add all other dep ? (stack, tabs, drawer, etc)

@Freddy03h
Copy link
Member Author

I didn't tried all the new props/options, but I'm pretty confident about the ones I removed from MaterialTopTabs

@MoOx
Copy link
Member

MoOx commented Jul 15, 2020

For the peer deps yes, please do what is recommended, so only what user should have as deps :) And all of them. Worst case scenario is that you get a warning for a dep you don't use, that's acceptable for now

As soon as you do that, I will adjust the version according to this... Since we don't "match" a single dep, the version number will be different that what we do for other packages.

@Freddy03h
Copy link
Member Author

Updated peerDependencies to this:

  "peerDependencies": {
    "@react-navigation/native": "^5.0.0",
    "@react-navigation/bottom-tabs": "^5.0.0",
    "@react-navigation/stack": "^5.0.0",
    "@react-navigation/material-top-tabs": "^5.0.0",
    "@react-navigation/material-bottom-tabs": "^5.0.0",
    "@react-navigation/drawer": "^5.0.0",

Regarding all packages changelog, it almost bugfixes, and features are almost typescript definitions and listeners things. There is a somes features linked to a specific minor version of a packaged but we don't supported them yet. So I think it's better to keep the 5.0.0 for the minimum version.

@MoOx
Copy link
Member

MoOx commented Jul 19, 2020

Your PR is still in Draft, do you have other things to push or is it ready as it is?

@Freddy03h Freddy03h marked this pull request as ready for review July 19, 2020 14:06
@Freddy03h
Copy link
Member Author

Nothing else to push for now ^^

@MoOx MoOx merged commit 49d5c87 into rescript-react-native:master Aug 11, 2020
@Freddy03h
Copy link
Member Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants