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

need to require PropTypes to use it #9095

Closed
wants to merge 1 commit into from

Conversation

willthefirst
Copy link
Contributor

Thanks for submitting a pull request! Please provide enough information so that others can review your pull request:

Unless you are a React Native release maintainer and cherry-picking an existing commit into a current release, ensure your pull request is targeting the master React Native branch.

(You can skip this if you're fixing a typo or adding an app to the Showcase.)

Explain the motivation for making this change. What existing problem does the pull request solve?

Prefer small pull requests. These are much easier to review and more likely to get merged. Make sure the PR does only one thing, otherwise please split it.

Test plan (required)

Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes UI.

Make sure tests pass on both Travis and Circle CI.

Code formatting

Look around. Match the style of the rest of the codebase. See also the simple style guide.

For more info, see the "Pull Requests" section of our "Contributing" guidelines.

@ghost
Copy link

ghost commented Jul 29, 2016

By analyzing the blame information on this pull request, we identified @caabernathy and @hramos to be potential reviewers.

@ghost
Copy link

ghost commented Jul 29, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 29, 2016
@ghost
Copy link

ghost commented Jul 30, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@mkonicek
Copy link
Contributor

Thanks for the pull request! Can you please mention NavigatorIOS in the title and update the description (which will become the commit message once this is merged)?

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 31, 2016
@willthefirst
Copy link
Contributor Author

require PropTypes explicitly

@willthefirst
Copy link
Contributor Author

done
On Sun, Jul 31, 2016 at 7:02 AM Martin Konicek notifications@github.com
wrote:

Thanks for the pull request! Can you please mention NavigatorIOS in the
title and update the description (which will become the commit message once
this is merged)?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#9095 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABD84uE0JAfyhQaz8-7n5dy2VL23CsCJks5qbKrpgaJpZM4JYqKa
.

@hramos
Copy link
Contributor

hramos commented Aug 1, 2016

Also, can you open this PR against master instead?

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 1, 2016
@willthefirst
Copy link
Contributor Author

created a new pull request on master: #9141

@willthefirst
Copy link
Contributor Author

in the future, if you want people to make their PR's on master, you should change the "Edit on Github" link in the docs. When I clicked it, it sent me here: https://github.com/facebook/react-native/blob/0.30-stable/Libraries/Components/Navigation/NavigatorIOS.ios.js

@hramos
Copy link
Contributor

hramos commented Aug 1, 2016

Good catch

@hramos
Copy link
Contributor

hramos commented Aug 1, 2016

That's something I'd like to fix, though I don't have a good solution for now. Many documentation pull requests are opened against the current release branch, but we actually want these to be opened against master to ensure the edits make it to the next release.

As you pointed out, the Edit on GitHub links point to the release branch and not master. This is expected because the doc site is versioned and built from release branches, and linking to master may produce broken links when files are renamed. We want to make it easy to find the source file used to generate any given doc page.

A quick fix would be to update these "Edit on GitHub" links to say "View on GitHub" instead, as our contribution guidelines already mention that PRs should be opened against master. I am not confident that this will mitigate the problem of new doc PRs being created against release branches, however.

Do you have any suggestions?

@willthefirst
Copy link
Contributor Author

Hmmm...you probably know more about this than I do.

I don't fully understand why just linking to the master branch would ever
produce broken links. Wouldn't the support site always be built from the
master branch?

On Mon, Aug 1, 2016 at 11:09 AM Héctor Ramos notifications@github.com
wrote:

That's something I'd like to fix, though I don't have a good solution for
now. Many documentation pull requests are opened against the current
release branch, but we actually want these to be opened against master to
ensure the edits make it to the next release.

As you pointed out, the Edit on GitHub links point to the release branch
and not master. This is expected because the doc site is versioned and
built from release branches, and linking to master may produce broken links
when files are renamed. We want to make it easy to find the source file
used to generate any given doc page.

A quick fix would be to update these "Edit on GitHub" links to say "View
on GitHub" instead, as our contribution guidelines already mention that PRs
should be opened against master. I am not confident that this will mitigate
the problem of new doc PRs being created against release branches, however.

Do you have any suggestions?


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#9095 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABD84j27QZ_WuoVHbCy6rh96_mxWMtuzks5qbjZtgaJpZM4JYqKa
.

@hramos
Copy link
Contributor

hramos commented Aug 1, 2016

A new documentation site is built for every release. If you look at the upper left of the window while browsing the docs, you'll see the current release number (0.30 as of this writing). If you click on that, you can choose a previous release, or select master/next to view the docs as they are on the master branch.

We don't display docs from master by default as they may document APIs or features that have landed in master but have not yet been cherry picked into a React Native release.

It's rare that we change links around (it happened most recently during our documentation lockdown at the end of June) so we may want to just link to master directly when clicking on Edit on GitHub, then.

ghost pushed a commit that referenced this pull request Aug 4, 2016
Summary:
We've been getting a lot of documentation PRs opened against `0.29-stable`, `0.30-stable`, and so on, instead of `master`. This is because our doc site is also based on RN release cuts, so clicking on the "Edit on GitHub" links on a document will take you to the markdown source for that release branch instead of the latest doc on `master`.

See #9095 for an example of such a PR.

In this PR we edit the link to say View on GitHub. Though it may not prevent PRs from being opened against a release branch, removing the "Edit" CTA may help in this regard.
Closes #9149

Differential Revision: D3664368

Pulled By: vjeux

fbshipit-source-id: 395c0813f736bfbe1be4b4fb1182f9060169365d
samerce pushed a commit to iodine/react-native that referenced this pull request Aug 23, 2016
Summary:
We've been getting a lot of documentation PRs opened against `0.29-stable`, `0.30-stable`, and so on, instead of `master`. This is because our doc site is also based on RN release cuts, so clicking on the "Edit on GitHub" links on a document will take you to the markdown source for that release branch instead of the latest doc on `master`.

See facebook#9095 for an example of such a PR.

In this PR we edit the link to say View on GitHub. Though it may not prevent PRs from being opened against a release branch, removing the "Edit" CTA may help in this regard.
Closes facebook#9149

Differential Revision: D3664368

Pulled By: vjeux

fbshipit-source-id: 395c0813f736bfbe1be4b4fb1182f9060169365d
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
Summary:
We've been getting a lot of documentation PRs opened against `0.29-stable`, `0.30-stable`, and so on, instead of `master`. This is because our doc site is also based on RN release cuts, so clicking on the "Edit on GitHub" links on a document will take you to the markdown source for that release branch instead of the latest doc on `master`.

See facebook#9095 for an example of such a PR.

In this PR we edit the link to say View on GitHub. Though it may not prevent PRs from being opened against a release branch, removing the "Edit" CTA may help in this regard.
Closes facebook#9149

Differential Revision: D3664368

Pulled By: vjeux

fbshipit-source-id: 395c0813f736bfbe1be4b4fb1182f9060169365d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants