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

docs: fix anchors in markdown doc file #1028

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

Conversation

artellador
Copy link


title: "docs: fix anchors in markdown doc file"

Anchors to headings were failing. After taking a look, I saw there were HTML anchor elements defined, but it seems after rendering the HTML, the "user-content-" prefix was being added.
With the proposed solution the links work but it is far from perfect, because it scrolls below the anchor. Enough as a quick fix, but can be improved. To make it better, probably we would need to replace all the anchors and use the ones generated by GitHub by default (that is, the heading text in lowercase, replacing spaces with "-").
My two cents to an interesting project :)


Related issue(s):


It seems GitHub adds it when rendering the HTML anchor elements
Anchors were not working because it was necessary to add them "user-content-" prefix
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

Copy link
Member

@derberg derberg 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 taking time to fix it

tbh, technically I think these a elements can actually be entirely removed as GitHub add these anyway, we just duplicate them. I have a theory we have these a elements as legacy from OpenAPI from old times when GitHub did not support autogeneration of a for headers

look, if you do this

## <a id="test"></a>Definitions

you will get this

<h2 data-sourcepos="87:1-87:31" dir="auto">
  <a id="user-content-definitions" class="anchor" aria-hidden="true" tabindex="-1" href="#definitions"><svg class="octicon octicon-link" viewBox="0 0 16 16" version="1.1" width="16" height="16" aria-hidden="true"><path d="REDACTED"></path></svg></a>
  <a id="user-content-test"></a>
    Definitions
</h2>

for few years GitHub was generating id without this additional text like user-content but then like...I think 2y ago they started doing it.

so yeah, I basically think the best would be just to entirely remove stuff like <a id="test"></a> from markdown headers cause we do not need them, GH generates them anyway. They follow approach to lower case and add - so Operation Bindings Object becomes user-content-operation-bindings-object but cool think is that internally the translate them and #operation-bindings-object also works like a charm

Also GH adds the chain icon on hover that you can click to get a link to the section
Screenshot 2024-02-13 at 16 06 35
and the link it generates, uses the GH generated href, not the one that we assume we control with custom a

@fmvilas do you remember maybe, is there a reason to keep these a elements other than inside tables, like <a name="messageExampleObjectHeaders"></a>headers? is it legacy from OpenAPI or some other thing?

@artellador
Copy link
Author

I though about rewriting all the anchors making use of the GitHub autogenerated anchors, but I didn´t want to remove elements explicitly added there, just fix what was broken.
Once you take a decision, if you opt for the autogenerated ones, if I have a gap, I´d make another PR.

@fmvilas
Copy link
Member

fmvilas commented Feb 22, 2024

@derberg this was done in the times when GitHub didn't support them in the headers (or they generated a different id). I think we can safely remove them. Let's make sure that the website doesn't rely on them. If that's the case, website should generate these itself instead.

@derberg
Copy link
Member

derberg commented Feb 22, 2024

@fmvilas thanks, yeah website generates its own ToC afaik.

@artellador you can go ahead and make proper adjustments

artellador and others added 12 commits February 24, 2024 17:38
First anchor update, running a program
Co-authored-by: Sergio Moya <1083296+smoya@users.noreply.github.com>

(cherry picked from commit cd29626)
Co-authored-by: Sergio Moya <1083296+smoya@users.noreply.github.com>%0ACo-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>

(cherry picked from commit f66ed79)
…ncapi#1032)

Co-authored-by: Sergio Moya <1083296+smoya@users.noreply.github.com>

(cherry picked from commit aed791e)
Reverted, pulled and applied anchor fixes
Copy link

sonarcloud bot commented Feb 24, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@derberg
Copy link
Member

derberg commented Apr 3, 2024

@artellador fyi, if you do not ping me (with a comment or rerequesting review) that the PR is ready for review - I will not know. Especially that you did few commits. So please specify whenever you are ready for final review

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

7 participants