-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Remove self-referencing links for webhooks docs #4283
Conversation
Fix for Issue readthedocs#4282; docs-only change.
docs/webhooks.rst
Outdated
to add. After you have added the integration, you'll see a URL for the | ||
integration on the :ref:`integration detail page <webhooks:Webhook Integrations>`. Use this | ||
URL when setting up a new webhook with your provider -- these steps vary | ||
integration once you go to readthedocs.org/dashboard, log in, and then go to |
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'm not sure if is the best to point to the users to that link and the log in step, I think if the users are reading this they already in their project's admin page (the above step already point to 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.
Agreed, good point. Revising.
docs/webhooks.rst
Outdated
integration once you go to readthedocs.org/dashboard, log in, and then go to | ||
**Admin** > **Integrations** to see or create an integration. | ||
|
||
As an example, the URL pattern looks like this: *readthedocs.org/api/v2/webhook/<project-name>/<nnnnnn>/*. |
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 can use <id>
instead of <nnnn>
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.
Thanks - and is that formatting okay with angle brackets and italic?
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, I think is fine
integrations, this integration has a specific URL, found on the | ||
:ref:`integration detail page <webhooks:Webhook Integrations>`. | ||
integrations, this integration has a specific URL, found on the project's | ||
**Integrations** Admin dashboard page on readthedocs.org. |
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.
Also here, probably the users already know that the project is in readthedocs.org, right?
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, I'm revising in the next patch, see what you think... I still find it a bit disorienting if I'm not in that Dashboard very often.
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 it is great now, we can change the wording here too. Thanks for the contribution btw ;)
docs/webhooks.rst
Outdated
@@ -90,8 +101,8 @@ branches | |||
Default: **latest** | |||
|
|||
token | |||
The integration token. You'll find this value on the | |||
:ref:`integration detail page <webhooks:Webhook Integrations>` page. | |||
The integration token. You'll find this value on the on your project's |
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.
on your
shouldn't be there
Looks good. Thanks for the PR! |
Fix for Issue #4282; docs-only change.