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

Proposed support for UUID anchor IDs #7

Closed
wants to merge 6 commits into from

Conversation

sadlerjw
Copy link

@sadlerjw sadlerjw commented Apr 9, 2016

This introduces a potential solution for displaying multiple articles/posts/whatever rendered by markdown-it to be displayed on the same page. For example, the index page of a blog may have the full text (including footnotes) of three or more articles, each separately rendered by separate calls into markdown-it. In this case, there will be a collision between footnote 1 of article 1 and footnote 1 of article 2.

This proposed solution introduces an option for using randomly generated anchor IDs, while still displaying regular numerals to the user, as before.

(This PR also includes an option for omitting square brackets in displayed footnote references, which is a requirement for my site...I'm happy to break it into a different PR or remove it entirely if you prefer.)

I know you've been against adding options in the past (I know someone proposed them to remove square brackets in #2) but I'd argue this is an important enough addition to add an option for it.

Happy to receive feedback on this. Thanks!

Jason Sadler added 6 commits April 8, 2016 18:37
@puzrin
Copy link
Member

puzrin commented Apr 9, 2016

IMHO it would be more universal to pass id prefix via env object. And random id is not very cool, because mutates output after every rebuild.

options are parser params (specific to parser instance), env is specific to document.

@sadlerjw
Copy link
Author

Good points. I'll go back and have a look.

@puzrin
Copy link
Member

puzrin commented Apr 10, 2016

In general:

  • Yes, problem is known, and should be solved.
  • Hash collisions (when multiple independent docs on the same html page) are not specific to this module. Would be nice resolve it global. See Header anchors [needs discussion] markdown-it#28 for other uses & problems.
  • I postponed this problem in hope to CM spec progress (about header anchors). But if it's painfull for footnotes - no problem, let's try to fix.

I think we had to invent a good property name for "document id" (to pass via env).

  • that will be needed anyway (and more universal than "id prefix/suffix").
  • that will be enougth to resolve problem in this module and will not break api.

Also, please, separate all other things (visual changes) for separate issues. Need to discuss first to find the best solution.

@sadlerjw
Copy link
Author

Ok, if I understand you correctly, we can go ahead with a prefixing solution in this module while we wait for something more general-purpose to surface in CM/markdown-it

@sadlerjw sadlerjw closed this Apr 10, 2016
@sadlerjw
Copy link
Author

Ugh submitted form accidentally. To continue, sounds like we just need to agree on a property name for defining an optional prefix in this plugin?

@sadlerjw sadlerjw reopened this Apr 10, 2016
@puzrin
Copy link
Member

puzrin commented Apr 10, 2016

I think, we need to define good doc id property name for env, because it's critical to have stable API. Then we can use is somehow in footnotes - that's local and not so critical.

@puzrin
Copy link
Member

puzrin commented Apr 10, 2016

I will need this feature too in nodeca, to display multiple posts in thread correctly. It was just not very urgent for me. But i'm ready to help with notes about architecture, and promise to accept proper implementation without delays.

I think, term "prefix" is not very good, because it can be suffix, for example. IMHO "document id" match better the root of problem. But i'm open to other suggestions.

@sadlerjw
Copy link
Author

Closing in favour of #8

@sadlerjw sadlerjw closed this Apr 11, 2016
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