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.

Sign up for GitHub

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

Jump to bottom

Use absoluteURL instead of relativeURL for next/prev links #783

Merged
merged 3 commits into from
Jun 17, 2018
Merged

Use absoluteURL instead of relativeURL for next/prev links #783

merged 3 commits into from
Jun 17, 2018

Conversation

endiliey
Copy link
Contributor

@endiliey endiliey commented Jun 16, 2018 •
edited
Loading

Motivation

Our docs prev/next button is build with relative path in mind. But relative path is really relative

If our current url is https://babeljs.io/docs/en/babel-plugin-transform-runtime
<a href='babel-register'/> will translate to https://babeljs.io/docs/en/babel-register

If our current url is https://babeljs.io/docs/en/babel-plugin-transform-runtime/
<a href='babel-register'/> will translate to https://babeljs.io/docs/en/babel-plugin-transform-runtime/babel-register which is wrong

The fix is to use absolute url 😄

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Before
Go to https://docusaurus.io/docs/en/installation/ (notice the ending slash /) and click previous/next button, 404 page
before

After
Go to http://localhost:3000/docs/en/installation/ (notice the ending slash /) and click previous/next button, no problem
after

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 16, 2018
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jun 16, 2018 •
edited
Loading

Deploy preview for docusaurus-preview ready!

Built with commit b35c2bd

https://deploy-preview-783--docusaurus-preview.netlify.com

@endiliey endiliey added the status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. label Jun 16, 2018
@endiliey endiliey changed the title Force extension-less url Remove trailing slash for docs Jun 16, 2018
@endiliey endiliey added ✅ Test Plan and removed status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. labels Jun 16, 2018
JoelMarcey
JoelMarcey approved these changes Jun 16, 2018
View reviewed changes
Copy link
Contributor

@JoelMarcey JoelMarcey left a comment

Choose a reason for hiding this comment

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

👍

Just one question.

lib/core/DocsLayout.js Outdated
@@ -47,7 +47,8 @@ class DocsLayout extends React.Component {
description={content.trim().split('\n')[0]}
language={metadata.language}
version={metadata.version}
metadata={metadata}>
metadata={metadata}
removeTrailingSlash={true}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would there be time when this would ever be set to false? Just wondering if we need a prop at all and we just always remove the trailing slash?

yangshun
yangshun reviewed Jun 16, 2018
View reviewed changes
Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Same question I have as Joel. I don't think this is the best way to fix though.

@endiliey I think the proper fix would be to generate absolute URLs for next/prev like what we do for the sidebar.

@endiliey endiliey changed the title Remove trailing slash for docs Use absoluteURL instead of relativeURL for next/prev links Jun 17, 2018
@endiliey
Copy link
Contributor Author

endiliey commented Jun 17, 2018 •
edited
Loading

@yangshun @JoelMarcey

Actually we always use relative url for next/prev in sidebar.

Docusaurus 1.2.0
https://github.com/facebook/Docusaurus/blob/3755522681bc5ea805be4aa6b06d5dfdf3226ec1/lib/core/DocsLayout.js#L84-L90

Docusaurus < 1.2.0
https://github.com/facebook/Docusaurus/blob/aee255219bedc97b55048cdb4703742cbb7c247e/lib/core/DocsLayout.js#L70-L73

I do feel my solution of removing trail slashes is a bit hacky though 😃. (I'm removing it)

Anyway, I'm changing it to use absolute URL with my latest commit so this bug will no longer exist 👍

@yangshun
Copy link
Contributor

Actually we always use relative url for next/prev in sidebar.

@endiliey Do we have next/prev in sidebar? What I mean is that in the left sidebar nav, we use absolute URLs, which is the approach I feel is better and we should do that for the next/prev buttons within the doc page.

@endiliey
Copy link
Contributor Author

endiliey commented Jun 17, 2018 •
edited
Loading

@yangshun Sorry. I think I misunderstood your comment before. I mean next/prev button in docs 😢

Anyway, changes updated. We now use absolute URL

yangshun
yangshun approved these changes Jun 17, 2018
View reviewed changes
Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@endiliey endiliey merged commit 0f01eac into facebook:master Jun 17, 2018
@endiliey endiliey deleted the cleanurl-redirect branch June 17, 2018 03:35
@endiliey endiliey mentioned this pull request Jun 17, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoelMarcey JoelMarcey JoelMarcey approved these changes

@yangshun yangshun yangshun approved these changes

Assignees
No one assigned
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Trailing slashes in link causing next/prev buttons to break
5 participants
@endiliey @docusaurus-bot @yangshun @JoelMarcey @facebook-github-bot