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

fix: wrong url in next/prev button in some cases #1488

Merged
merged 1 commit into from
May 18, 2019

Conversation

hong4rc
Copy link
Contributor

@hong4rc hong4rc commented May 18, 2019

Motivation

Resolved #1344

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

  • I clone https://github.com/reduxjs/redux and install all dependencies
  • Use this commit for node_modules/docusaurus
  • Run yarn start
  • Go to http://localhost:3000/faq
  • Go to the bottom of the page and click on "General" and it goes to http://localhost:3000/faq/general
  • Go to the bottom of the page and click on "FAQ Index" and it goes to http://localhost:3000/faq (it goes to http://localhost:3000/ without this commit)

You can see the preview in https://hongarc.github.io/redux/faq

@hong4rc hong4rc requested a review from endiliey as a code owner May 18, 2019 02:43
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 18, 2019
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-2 ready!

Built with commit a4a64a8

https://deploy-preview-1488--docusaurus-2.netlify.com

@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-preview ready!

Built with commit a4a64a8

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

@endiliey
Copy link
Contributor

@hongarc yes u can remove that line.

@hong4rc
Copy link
Contributor Author

hong4rc commented May 18, 2019

yes u can remove that line.

Sorry, that is my fault, if I remove that line, .replace(/^\.\.\//, '') can't find ../ (in windows)

@endiliey endiliey changed the title fix: wrong relativeHref fix: wrong relativeHref in next/prev button May 18, 2019
@hong4rc
Copy link
Contributor Author

hong4rc commented May 18, 2019

And I have one way is:

--- .replace('\\', '/')
--- .replace(/^\.\.\//, '')
+++ .replace(new RegExp(`^\\.\\.\\${path.sep}`), '')

It is shorter. Are you want to use it?

@endiliey
Copy link
Contributor

Nah, keep it as it is. Shorter doesn't mean it's more declarative.

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

LGTM

Good trick of appending html to make sure faq and faq/general relativeHref is correct

Previously it will be general but after this PR it become ../faq/general.html

@endiliey
Copy link
Contributor

🚀 Ship it ....

shipit

@endiliey endiliey merged commit 1587810 into facebook:master May 18, 2019
@endiliey endiliey changed the title fix: wrong relativeHref in next/prev button fix: wrong url in next/prev button in some cases May 18, 2019
@hong4rc hong4rc deleted the fix/relative-href branch May 18, 2019 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redux docs next page link not working
4 participants