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

Use relative links and translate internal references #603

Merged
merged 2 commits into from
Jul 11, 2018

Conversation

cetra3
Copy link
Contributor

@cetra3 cetra3 commented Feb 2, 2018

So this PR is a breaking change. This converts everything to a relative URL, and also resolves internal links, solving #408.

This does two things:

Making all the links relative within the output html

Currently the html output relies heavily on the <base href for links. This PR basically removes this from the template, and converts all links to relative, adding in the appropriate path_to_root variable.

Removing this has the following benefits:

  • Images work on non-root URLs I.e, http://example.com/book/.
  • A lot of workarounds can be stripped out: anchors and the index page come to mind.
  • It enables the ability to convert internal links as per below

Converting internal links from .md to .html

This is the second part of the change, and does break the internal structure of some books, but fixes #408.

As an example in thebook-example dir on the init.md page, the following link:

[chapter](format/summary.html)

Is now changed to:

[chapter](../format/summary.md). 

This has the following benefits:

  • Viewing this new file on github, you can click on it and it will go to that file, rather than giving you a 404
  • This extends to editors such as vscode, which can follow links as per normal
  • This also extends to other markdown generators like mkdocs, which use this method already.
  • It keeps the links consistent to what you are writing in SUMMARY.md.

@sergicolladosopra
Copy link

It would be nice to have internal links conversions ... I just was looking for this in the documentation, a simple way to link internal pages, if I link with .html we lose repository navigation, if I link with .md we lose web navigation :(

@weihanglo
Copy link
Member

weihanglo commented May 1, 2018

What state is this issue in? Is it still waiting for reviews?

It would be helpful to write relative markdown links instead of directly links to HTML. If this feature works with #685, the behavior would be more consistent accorss GitHub/Bitbucket and some other website. As @cetra3 mentioned above, no more 404 page and README.md is served as index file.

@cetra3
Copy link
Contributor Author

cetra3 commented May 1, 2018

@weihanglo yep, still waiting for a review.

As there have been some changes to the master I may need to re-cut this, if the appetite is there.

@Michael-F-Bryan
Copy link
Contributor

I was initially a little hesitant about introducing the breaking change and complicating the Book type a bit more. After merging #685, I'm feeling like switching to use relative links everywhere is the way to go. It should hopefully mean you break less stuff while rearranging books and being able to jump between links when viewing the book's source code on GitHub (or similar UIs) will be super useful.

It's a breaking change though, so we may need to put together a quick link rewriting script to make migration easier. The Book uses mdbook and would probably be the most adversely affected, @steveklabnik or @carols10cents, do you have any opinions?

Sorry for leaving this so long @cetra3! Are you still interested in completing this PR?

@cetra3
Copy link
Contributor Author

cetra3 commented May 11, 2018

Yes. Definitely

@Michael-F-Bryan
Copy link
Contributor

@cetra3 awesome!

It should hopefully be easy enough to git rebase this PR onto master, but if there are a lot of merge conflicts it may be less hassle to start a new PR and use this one as a rough guide. How does that sound?

Looking through the PR, to translate from absolute links to relative links we'll need to:

  • Add some new integration tests to make sure we render the correct relative links
  • Insert a step to change the extension of all *.md links to *.html
  • Add a path_to_root variable to the template's render context
  • Update all the templates to prepend any link to a static asset with path_to_root
  • Make sure everything renders correctly and links all work

Did I miss anything from that list?

If there's anything we can do to help, please let us know!

@Michael-F-Bryan Michael-F-Bryan removed their request for review May 14, 2018 10:59
@cetra3
Copy link
Contributor Author

cetra3 commented May 14, 2018

I will see how easy it is to rebase rather than resubmit it. Should there be some sort of migration path for existing projects?

@Michael-F-Bryan
Copy link
Contributor

Michael-F-Bryan commented May 16, 2018

Should there be some sort of migration path for existing projects?

I'm not sure. When this lands we'll do a major version bump and post something to both the user forums and /r/rust on Reddit, hopefully that should be enough.

@weihanglo
Copy link
Member

Is here anything that I can help?
I am very looking forward to this PR and the removal of base tag at #279.

@cetra3
Copy link
Contributor Author

cetra3 commented Jun 1, 2018

@weihanglo It's a WIP on a local branch. I'm just doing a few tests to get it to work with the new version.

@cetra3
Copy link
Contributor Author

cetra3 commented Jun 1, 2018

@Michael-F-Bryan it's now basically rebased from the current master, so no merge conflicts. Had to adjust a couple more tests as they were failing.

@cetra3
Copy link
Contributor Author

cetra3 commented Jun 1, 2018

@weihanglo you can install this PR via cargo with:

git clone https://github.com/cetra3/mdBook --depth 1 -b relative_links && cargo install --path mdBook --force

@chadmorrow-fd
Copy link

What are the next steps before this can be merged?

@cetra3
Copy link
Contributor Author

cetra3 commented Jul 10, 2018

I am just waiting for @Michael-F-Bryan to take it from here and get the changes merged.

@Michael-F-Bryan Michael-F-Bryan merged commit bdb37ec into rust-lang:master Jul 11, 2018
@Michael-F-Bryan
Copy link
Contributor

Sorry for the long wait @cetra3, I've been busy with work and not really had any time for my personal projects.

I checked this out locally and as far as I can tell there aren't any issues. Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mdbook should translate internal references
5 participants