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

add support for links with anchors from summary, issue #167 #1128

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tglman
Copy link

@tglman tglman commented Jan 22, 2020

As the title discribe

@tglman tglman requested review from ehuss and Dylan-DPC-zz January 27, 2020 17:27
@tglman
Copy link
Author

tglman commented Feb 4, 2020

Hi all,

Any chance to have a review of this ?

Regards

@ehuss
Copy link
Contributor

ehuss commented Mar 9, 2020

I can't get this to work. Adding foo.md#bar just creates a file src/foo.md#bar, and the sidebar doesn't work at all. How is it supposed to work?

Also, how are you addressing all of the concerns listed at #167 (comment)? It seems like it's a pretty major shift to mdBook's understanding that every chapter is a single file.

@tglman
Copy link
Author

tglman commented Mar 9, 2020

Hi @ehuss

Thanks for your feedback, so the check for the file and the anchor are done here: https://github.com/rust-lang/mdBook/pull/1128/files#diff-3764eab707e02a92f9da67865a327055R237

So if the file "src/foo.md" do not exists for now it consider the "foo.md#bar" as the file name, i think this is happening in your case, let me know if you prefer a different behaviour, I've done this in this way to keep the behaviour compatible do previous the change.

for the concern you linked:

I wouldn't want to allow arbitrary links in the summary, even the proposed page-title.md#section-header could easily break the hierarchy. If added I think we should go with #section-header alone and it would automatically refer to the parent chapter.

I didn't handle this.

There are some edge cases the parser would have to worry about. For example: Section links can't contain sub-chapters, they also can't be at the root level.

I didn't handle this.

The current implementation assumes all links from the summary are files. We could add some dirty checks to make it work, but if we want to do it properly, the concerned parts should be rewritten to support it.

I did handle this, if the link contains an anchor and the file name is rewritten without the anchor and the anchor is stored in the Chapter structure to generate afterward the TOC.

I will do a double check of it, I'm currently testing this on a existing doc here: https://github.com/orientechnologies/orientdb-docs/commits/mdbook

Let me know as well if you prefer that I handle this in a different way.

Regards

@tglman
Copy link
Author

tglman commented Mar 9, 2020

Hi @ehuss,

Just to make you aware, I noticed that because I modified the Chapter structure, that is serialized and deserialized for be passed to the pre-processors, if you have a pre-processor compiled with the old version of mdbook will remove the "anchor" because in the old version of the json format used to pass informations they are not present.

Not sure how this could actually be fixed in a generic way, suggestion are welcome here.

Regards

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.

2 participants