Skip to content

Add toc link when enable set to true. #69

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

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

Conversation

zw963
Copy link
Contributor

@zw963 zw963 commented Feb 11, 2025

Enable display a link svg when set Markd::Options.new(toc: true), like following screenshot.

image

@icyleaf
Copy link
Owner

icyleaf commented Mar 2, 2025

Anchor link is a part of toc, full toc feature needs to generate the table of content or provide a method to get the toc.

@zw963
Copy link
Contributor Author

zw963 commented Mar 2, 2025

generate the table of content

Yes, this is one of feature of toc, but i thought it's optional.

But, i consider anchor link is necessary for a HTML rendered markdown title.

@nobodywasishere
Copy link
Collaborator

I think this is something best implemented in a custom renderer rather than upstream

@zw963
Copy link
Contributor Author

zw963 commented May 9, 2025

I think this is something best implemented in a custom renderer rather than upstream

Yes, I use it on my fork, if we make the § string configurable, it is PR acceptable?

@nobodywasishere
Copy link
Collaborator

I'm not sure. On one hand this is useful; on the other this isn't something specified in the commonmark or gfm specs, which leads me towards it being something implemented by those who want it. How do we feel about out of spec extensions?

@straight-shoota
Copy link
Contributor

I think anchor links for every heading are a very valuable UX feature.

However, it's a bit tricky getting the HTML right. The current implementation for example creates an anchor element adjacent to the header element. But they're not directly grouped together, which can make it hard to style properly.
A commonly used structure wraps them into a div:

<div class="header-wrapper">
  <h2 id="title">Title</h2>
  <a href="#title">§</a>
</div>

Improving accessibility requires some further enhancements to make sense of the anchor link.

IMO a decent default implementation could very well fit into upstream. But it should also be easily customizable with custom renderers.

So maybe it's best to start with that to make sure it's simple to override the details without changing the overall structure?

@zw963 zw963 force-pushed the add_toc_link branch 3 times, most recently from d97572b to 0715608 Compare May 11, 2025 12:42
@zw963
Copy link
Contributor Author

zw963 commented May 11, 2025

Maybe we can add a new option instead of toc for enable this PR feature, leave the toc only for original topic purpose.

Copy link
Owner

@icyleaf icyleaf left a comment

Choose a reason for hiding this comment

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

Missing unit tests, add it pls.

@trafico-bot trafico-bot bot added ⚠️ Changes requested Pull Request needs changes before it can be reviewed again and removed 🔍 Ready for Review Pull Request is not reviewed yet labels May 12, 2025
@trafico-bot trafico-bot bot added the 🔍 Ready for Review Pull Request is not reviewed yet label May 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ Changes requested Pull Request needs changes before it can be reviewed again 🔍 Ready for Review Pull Request is not reviewed yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants