erusev / parsedown-extra

Markdown Extra Extension for Parsedown
MIT License
817 stars 121 forks source link

Unique Footnote anchors #50

Open graphidev opened 9 years ago

graphidev commented 9 years ago

As I am using Parsedown-extra many times in a same page, the anchors target not only the concerned item. E.g:

` Item 1

fn:1

fnref1:1

Item 2

fn:1

fnref1:1

`

As you can the see, this always refer to a same item footnote (the first one). So is it possible to generate an unique id for footnotes ?

erusev commented 9 years ago

I believe it is possible.

In fact, it shouldn't be hard to implement.

Would anyone be willing to create a PR?

graphidev commented 9 years ago

Wait, I just thought that footnotes could serve as permalinks. So they shouldn't have to be unique (as PHP uniq built-in function) at each Parsedown-extra parsing.

I will think about this and make a PR

erusev commented 9 years ago

So they shouldn't have to be unique (as PHP uniq built-in function) at each Parsedown-extra parsing.

I'm not sure I understand.

My solution would be to keep the footnote data in a class (static) variable. This would make sure that the numbers of footnotes are unique among different instances of the parser.

graphidev commented 9 years ago

My bad.

I was thinking of a blog architecture where posts are ordered by descending date.

The fn1:1 would target the first foot note of the first post. However, when a new post is available, then the permalink http://example.com/#fn1:1 will no more refer to the right post (should refer to the second one).

Maybe this could be available as an option to set a custom runtime id. Then the foot notes ids could be something like post456-fn1:23

graphidev commented 9 years ago

I think the fix made by @robinadr (a234bbeb4d25fe9bb6d40ceb3df0945f981636f7) is a good answer to this probleme. Could it be merged with parsedown-extra in order to get the update from Composer ?

erusev commented 9 years ago

Thanks, I'll have a look as soon as I have some time.

robinadr commented 9 years ago

I'm debating whether to either add a clearFootnotePrefix() method or make it automatically clear after each text rendering. That being said, it doesn't seem any of the other setters are one time use only.

Anybody have any feedback on this?

graphidev commented 9 years ago

It would be weird to use twice a same ID (or prefix, as you want) :

I think, in my personal opinion, that the prefix must be reseted after each text rendering. However, someone could have an other example that goes to the opposite.

robinadr commented 9 years ago

I agree. Given that this is a new feature, we have an opportunity now to change the behavior as we imagine it would work best.

Another more generic option is what was previously suggested: keep an internal count and add that on as necessary.

This is simpler but I think it fails when someone might create multiple instances of the parser for a single web page load.

I think having the prefix reset is a good idea, but I'm also wondering if that should be able to be toggled on or off. But then we run the risk of over complication.

On Apr 2, 2015, at 10:37 AM, Sébastien ALEXANDRE notifications@github.com wrote:

It would be weird to use twice a same ID (or prefix, as you want) :

There is no reason to parse twice the same text Parsing various texts with a same ID could generate conflicts (which is the reason of this issue). I think, in my personal opinion, that the prefix must be reseted after each text rendering. However, someone could have an other example that goes to the opposite.

— Reply to this email directly or view it on GitHub.

graphidev commented 9 years ago

You are right, both generic and custom prefix should be implemented.

Here are the behaviors I suggest :

<?php

    $parsedown = new ParsedownExtra;

    $parsedown->text('...');  # 1
    $parsedown->text('...');  # 2

    $selfReset = true;
    $parsedown->setFootnotePrefix('abcd1234', $selfReset);
    $parsedown->text('...');  # abcd1234

    # if $selfReset is FALSE, then call
    # $parsedown->resetFootnotePrefix(/* FALSE by default */) 
    # in order to reset the custom prefix

    $parsedown->text('...');  # 3 : Previous custom prefix does not reset generic one
    $parsedown->text('...');  # 4

    # Reinstanciated ParsedownExtra does not use previous generic prefix
    $anotherParsedown = new ParsedownExtra;
    $anotherParsedown->text('...'); // # 1

    # reset both generic and custom prefix
    $resetBothCustomAndGeneric = true;
    $parsedown->resetFootnotePrefix($resetBothCustomAndGeneric);
    # private $genericPrefixCount = 0
    # private $customPrefixCount = null

    $parsedown->text('...');  # 1
    $anotherParsedown->text('...');  # 2 : not affected by the first instance reset

    # ...

?>

Keeping generic prefix in memory for all the Parsedown instances may generate some conflicts, such as incrementing count for $anotherParsedown instance when it should not.

robinadr commented 9 years ago

I feel that's going a little too far. Parsedown is a backend library, so I think it's fair to assume the developer using it can be held responsible to prefix footnotes as necessary. This all depends on the processing of the page (especially multiple parsing passes to be displayed on the same page) as well as how they've worked their code (same parser instance or multiple).

With all these factors dependent on the developer, I think it's most flexible to add a prefixing capability, then let the developer deal with it how they want.

On Apr 3, 2015, at 6:13 AM, Sébastien ALEXANDRE notifications@github.com wrote:

You are right, both generic and custom prefix should be implemented.

Here are the behaviors I suggest :

<?php

$parsedown = new ParsedownExtra;

$parsedown->text('...');  # 1
$parsedown->text('...');  # 2

$selfReset = true;
$parsedown->setFootnotePrefix('abcd1234', $selfReset);
$parsedown->text('...');  # abcd1234

# if $selfReset is FALSE, then call
# $parsedown->resetFootnotePrefix(/* FALSE by default */) 
# in order to reset the custom prefix

$parsedown->text('...');  # 3 : Previous custom prefix does not reset generic one
$parsedown->text('...');  # 4

# Reinstanciated ParsedownExtra does not use previous generic prefix
$anotherParsedown = new ParsedownExtra;
$anotherParsedown->text('...'); // # 1

# reset both generic and custom prefix
$resetBothCustomAndGeneric = true;
$parsedown->resetFootnotePrefix($resetBothCustomAndGeneric);
# private $genericPrefixCount = 0
# private $customPrefixCount = null

$parsedown->text('...');  # 1
$anotherParsedown->text('...');  # 2 : not affected by the first instance reset

# ...

?> Keeping generic prefix in memory for all the Parsedown instances may generate some conflicts, such as incrementing count for $anotherParsedown instance when it should not.

— Reply to this email directly or view it on GitHub.

graphidev commented 9 years ago

My bad, I didn't read correctly your previous comment.

So let's take care of this part only :

$selfReset = true;
$parsedown->setFootnotePrefix('abcd1234', $selfReset);
$parsedown->text('...');  # abcd1234

# if $selfReset is FALSE, then call
# $parsedown->resetFootnotePrefix(/* FALSE by default */) 
# in order to reset the custom prefix
robinadr commented 9 years ago

I’m still inclined to let the developer take care of it. We can add a resetFootnotePrefix() method if necessary (it would just be setFootnotePrefix(‘’)) but my general philosophy is to leave control up to the developer using the library. It might mean an extra line or two for them, but at the end of the day I value the flexibility. That’s just me, though.

robinadr commented 9 years ago

I pushed a change that adds clearFootnotePrefix() for now.

graphidev commented 9 years ago

I just agree with your philosophy. I apologize if my samples wasn't looking that way.

Thanks a lot for you work !

robinadr commented 9 years ago

No worries, just trying to figure out what the best approach is before it's potentially pushed into the main repo. Waiting to hear what @erusev wants to do.

erusev commented 9 years ago

@robinadr I've been super busy lately. I'll respond as soon as have some time.

wladston commented 8 years ago

Can we merge this in already? :)

erusev commented 8 years ago

could someone experiment w/ the original Markdown Extra and see if this issue is addressed there