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

Fixed the display of include:: links for github and a broken link #173

Merged
merged 2 commits into from
Jan 3, 2018

Conversation

obilodeau
Copy link
Member

Broken link found while reviewing #169, a 1.1.0 regression.

@mojavelinux I would like your opinion on the way I workaround GitHub's include:: are turned into link: behavior.

Broken link found while reviewing asciidoctor#169, a 1.1.0 regression.
@mojavelinux
Copy link
Member

What I recommend is just making a regular link when the GitHub environment is detected. No need to use an include since it won't get interpreted anyway.

ifeval::[{safe-mode-level} >= 20]
See <<examples/basic-example.adoc#,basic-example.adoc>>.
endif::[]
ifeval::[{safe-mode-level} < 20]
.basic-example.adoc
[source,asciidoc]
....
include::examples/basic-example.adoc[]
....
endif::[]

Technically it's better to check the safe mode level since that's what determines whether includes are enabled or not.

@obilodeau
Copy link
Member Author

obilodeau commented Jan 3, 2018

I made the suggested changes.

One thing though, a link to an adoc file on GitHub will display its converted version not the source. I think this could be confusing to users. We could link to a /raw/ link (example) but then the HTML version would no longer work offline as the link would no longer be relative.

Thoughts?

Edit: Worse than that, we would need to point to a specific branch or tag otherwise links could be broken if we ever move things around.

@obilodeau obilodeau merged commit a52639b into asciidoctor:master Jan 3, 2018
@obilodeau obilodeau deleted the doc-fix-github-links branch January 3, 2018 04:32
@obilodeau
Copy link
Member Author

I created #176 to have that discussion since it is not release-critical and we need to do a release $soon.

@mojavelinux
Copy link
Member

👍

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.

None yet

2 participants