-
-
Notifications
You must be signed in to change notification settings - Fork 521
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
chore: update contributing guide #2140
Conversation
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.
Great stuff! Left my review below 👇
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
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.
My initial impressions. I'll reiterate on this PR
guides/CONTRIBUTING.md
Outdated
|
||
For commits and pull request names we follow a [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) specification. | ||
> [!WARNING] | ||
> Those steps are crucial, if you change the API in react-native-screens and won't merge react-navigation changes the libraries may go out of sync and crash i.e. because of not existing property. On the other hand, if you don't perform step 6 and 7, test examples and showcase app may stop working in react-native-screens. |
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 is a bit exaggerated. Any backward compatible changes in react-native-screens
, so changes in minor
/ patch
versions should go unnoticed if react-navigation
is not updated. Only breaking changes would break the behaviour and we're fine with that.
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.
So my idea was that the previous sections let you know if you need or do not need to open PR to react navigation. And if you need, you end up in this section. Then those steps are crucial (?). But I can rephrase that if it's not clear.
Co-authored-by: Kacper Kafara <kacper.kafara@swmansion.com>
Co-authored-by: Kacper Kafara <kacper.kafara@swmansion.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com>
## Description Our contributing guide was very basic and written long time ago. I updated it with some more insights and details on how to start contributing to this project. --------- Co-authored-by: Tymoteusz Boba <Tymoteusz.Boba@gmail.com> Co-authored-by: Kacper Kafara <kacper.kafara@swmansion.com>
Description
Our contributing guide was very basic and written long time ago. I updated it with some more insights and details on how to start contributing to this project.