Open sneakers-the-rat opened 1 year ago
Looks good! One thing I'd be concerned about is that the prefix/suffix used by the citation style could be blank or also occur within the actual citation. One (admittedly hacky) way around this could be to temporarily add a unique prefix/suffix to the style prior to rendering it. Alternatively you could add the anchor tag itself to the prefix/suffix so that the links are rendered by the cite processor itself.
Gotcha. yeah you have a better sense of the variation there. I think the regex should be able to handle that - i assume if there are no prefix/suffix the attributes would just be an empty string rather than a NULL? but i can do an explicit check for that. The regex should only sub the prefix/suffix if it occurs at the beginning/end of the generated string, but ya i should also test that specifically.
I had also thought about just making a citation style that does that explicitly, but it seems like that would be pretty fragile, and this seems like a general improvement to all citation styles (i can't think of a case where it would be preferable to only link to the first citation vs. each has their own link? but having worked around it I totally get why it works that way given the rest of the structure of the packages) if it can work reliably?
Also I wanted to separate the parsing of the citation text and the wrapping of the link so that I could add unique ids to each of the in-text citations for backlinks, which will be the next step :)
My apologies, I completely missed that you were already rendering the citations individually. So, yeah, you're right about matching prefix/suffix at the end and beginning etc. that should be fine.
You'd have to check but one potential downside here is that there are some features of citation styles that we could be missing this way (e.g., some styles would use something like 'ibid' for consecutive citations or use disambiguation like adding letters to years) but these may already be disabled by the way the citations are used by jekyll-scholar anyway. So I think that's fine too. What I like about your approach is that it works with any style.
I think you could just clear the prefix/suffix in the style actually, render the citation, add the link, and then wrap it the way you already do. That is, you could skip stripping the prefix/suffix in the first place?
ha i was nervous about modifying the style itself, but if you think that would be a better way to do it then i'm happy to swap it to that. I should really just write tests that confirm these don't break the rendering across citation styles, it seems like that might be a useful addition to the tests generally?
Sounds great. In fact, tests are sort of mandatory for a PR, because I have not been using jekyll-scholar myself for years. Besides me, there are a handful of users who contributed significant features and who also help maintaining from time to time, but basically, if someone wants to add a feature, make a change or fix something we rely heavily on the tests to catch regressions.
Hello! I'm working my way towards implementing https://github.com/inukshuk/jekyll-scholar/issues/344 and to do that I wanted to make each of the in-text references have their own individual link.
I made one pretty janky attempt here (that should also cache the work done in rendering the citation to html) by rendering the text version of each citation, stripping the prefix and suffix, rendering the HTML with the link, and then recombining at the end. It works, but probably not how you would do it. Just want to see if you'd be interested in something like this as a PR and how you would have done it differently so i can make it better than it is now which i imagine will be buggy:
https://github.com/sneakers-the-rat/jekyll-scholar/commit/4a3bc1f9e2fff956ff585d2b8c14d2d825b5ca5b