-
Notifications
You must be signed in to change notification settings - Fork 52
fix broken links in the “See Also” section #81
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
Conversation
…p#74) The links in the “See Also” section on topic pages lead to a 404. This is very annoying since RDocumentation is often the first result for searches related to R packages. The missing page is: `/link/:topic?package=:package&version=:version` It can’t be fixed by simply redirecting to `/packages/:package/versions/:version/topics/:topic` because the function names in the “See Also” section don’t always map directly to topic pages. There is an API endpoint with the same path that resolves the correct topic page and fallbacks with a search results. `[API_URL]/link/:topic?package=:package&version=:version` https://github.com/datacamp/RDocumentation-app/blob/aa71a42af087a850df69d5c7ecbc01316cc31f0e/api/controllers/TopicController.js#L263-L367 The problem is, it redirects to a page in the API domain, instead of returning JSON with a correct relative URL. `[API_URL]/packages/:package/versions/:version/topics/:topic` The solution: create the link page and fetch the corresponding API endpoint, get the URL it redirects to from response.url, strip the domain name and redirect. This can be improved further by adding an API endpoint that returns the correct URL or by resolving the URL when the link is created.
|
thank you for this fix hope it'll be merged soon |
|
Hi, @ncarchedi, @ddmkr can I expect this to be merged? It's a non breaking change that greatly improves the user experience. |
|
@ztsorojev ? I saw you merged something recently, is there a reason not to do this? Usability is seriously impaired. |
|
@sin Thank you for your contribution!! 🙏 Sorry that I replied so late! And thanks @ariliso for tagging me! |
|
@ztsorojev Thank you! It also fixed the links in the body of text, not only the “See Also” section. I've found out that we can sanitize the input to make it work more often. For example, this link doesn't work: Dropping the brackets from the function name would fix a lot of links. I'll create another PR shortly. |
Some of the links are formatted as `function_name()` or `library::function_name()` instead of just `function_name`. This is the convention in the tidyverse packages' docs. This change simply removes the brackets and splits the library and function names. Link to `tidyselect::vars_pull()` from tidyr: Before: /link/tidyselect::vars_pull()?package=tidyr After: /link/vars_pull?package=tidyselect
The links in the “See Also” section on topic pages lead to a 404. This is very annoying since RDocumentation is often the first result for searches related to R packages. The missing page is:
/link/:topic?package=:package&version=:versionAnd it should point to:
/packages/:package/versions/:version/topics/:topicHowever, It can’t be simply fixed by redirecting to the correct URL because the function names in the “See Also” section don’t always map directly to topic pages. There is an API endpoint with the same path that resolves the correct topic page and fallbacks with a search results:
[API_URL]/link/:topic?package=:package&version=:versionThe problem is, it redirects to a page in the API domain, instead of returning JSON with a correct relative URL.
[API_URL]/packages/:package/versions/:version/topics/:topicThe solution: create the link page and fetch the corresponding API endpoint, get the URL it redirects to from response.url, strip the domain name and redirect.
This can be improved further by adding an API endpoint that returns the correct URL or by resolving the URL when the link is created thus saving on two redirects.
This solves #65 and #74.