markdown-it / markdown-it-footnote

Footnotes plugin for markdown-it markdown parser
https://markdown-it.github.io/
MIT License
210 stars 59 forks source link

Add configurable footnoteId and footnoteCaption renderers #9

Closed chromakode closed 8 years ago

chromakode commented 8 years ago

Similar to #8, I am wrestling with footnote ids for multiple markdown bodies in the same page. In addition, I want the hash URLs for footnotes to be stable if new footnotes are added. This proposal would allow you to customize the id and caption output by writing your own renderer.

For instance, to generate stable footnote ids using labels (when available):

require('markdown-it')({
  footnoteId: function(n, token) {
    if (token.meta.label) {
      return '-' + token.meta.label;
    }
    return n;
  },
}).use(require('markdown-it-footnote'))

Similarly, one could implement a footnoteId function which includes a per-document id.

One downside to this approach is that a custom renderer could introduce security issues since it's output directly into the HTML. Do you have any other ideas for mitigating this, or do you think it's ok as-is? One idea would be to escape the custom renderer output as a fail-safe.

rlidwka commented 8 years ago

First of all, the arguments are passed to the plugin incorrectly.

Please check out readme for markdown-it-emoji if you want to learn more about that.

Also, I believe footnoteId doesn't work in this commit. Tried it out, the function wasn't called.

I'm going to close this for now. I believe we're going to merge #8 over this weekend, and this PR will be in conflict after that.

If you still think id and caption should be configurable, please open a separate issue for bikeshedding purposes to discuss how best to implement it.

chromakode commented 8 years ago

I hope that you will reconsider. This approach satisfies the same requirement as #8, and helps avoid future bikeshedding on this topic by making these functions customizable. I don't think there's a one-size-fits-all choice for footnote ids and captions, so it is valuable to provide flexibility.

It is simple to configure a prefixed id like in #8:

footnoteId: function(n, token) {
  // generate ids of the form #fn-mydocument-1, #fn-mydocument-2
  return '-mydocument-' + n;
}

But it also allows you to use this flexibility to generate semantic ids, which won't change as footnotes are reordered or the document changes. This is very important if the anchors will be linked to.

footnoteId: function(n, token) {
  if (token.meta.label) {
    // if a footnote has a name, generate ids of the form #fn-footnote-label
    return '-' + token.meta.label;
  }
  // otherwise, #fn1, #fn2
  return n;
},

Did the tests work for you? They exercise example footnoteId and footnoteCaption functions and verify that they are working.

It should be simple to update the code to take options from the .use(..., /* here */) format you described. I was trying to make this easy to use in combination with markdown-it-loader, but it sounds like the right solution is to make changes to markdown-it-loader to allow for options objects to be passed to the plugins.

rlidwka commented 8 years ago

Also, I believe footnoteId doesn't work in this commit.

My mistake here, apparently I was trying to render invalid markup.

I'm trying to fix options and merge both this PR and #8 (since one is simpler and another is more configurable, it kinda makes sense to offer both). No idea how it would work out.

Reopening for now.

rlidwka commented 8 years ago

Okay, so we've decided that it's easier for people to define anchor like this:

var md = require('markdown-it')().use(require('./'))

//md.renderer.rules.footnote_anchor_name = function (tokens, idx, options, env) {/*...*/};
md.renderer.rules.footnote_caption = function (tokens, idx, options, env) {
  var n = Number(tokens[idx].meta.id + 1).toString();

  return '{' + n + '}';
};

console.log(md.render('[^1]\n\n[^1]: whatever'))

So that's how it's been implemented.

If you have any questions or suggestions or objections, please ask @puzrin. I've implemented this change, but I have no opinion on it and don't care either way.