Added anchor links to admonition blocks for direct referencing #2676
Conversation
|
Won't this be rather brittle, though? I.e. if another admonition is inserted or removed before the linked on, it will silently break the link (or rather the link will end up pointing to something different than intended). |
|
Though of course that's fine for "throw away linking ", eg when sending someone in Slack a link so they find something more quickly. But for stable links eg between one part of a manual to another it'd be better if there was a way to specify an anchor name. |
|
All that said, I don't really mean to be a buzzkill, sorry for that, and thank you for working on this! |
|
you're right. now I think of it I think we can add syntax for manually assigning IDs to admonitions that need to be referenced like we are doing with headers like [Header] (#id) . do you think that's a good idea? |
absolutely not, I agree with your point of this approach being brittle, thank you for pointing that out! |
|
One super-minor style nitpick: I think it would be nice if the link icon was the same size as the existing heading icons:
It's a bit tricky with admonitions actually -- syntax-wise, there is no way to add "silent" metadata (like you can with the We already create the slug based on the admonition title, if present. So if the user gives their admonitions titles, then we're less likely to run into this issue (although I guess admonition titles could also be repetitive in some cases). And OTOH, we have similar brittleness issues with headings and docstrings. I guess one option would be to create the (fallback) slug based on a hash of the content of the admonition block. That should ensure that it would only change if the content changes, and ensure that you don't get linked to the wrong admonition. But I'd also be fine with this as is to be honest, as it's already an improvement. |
|
I was thinking of something like this : thoughts? |
|
I have fixed the link size, if you think this is good enough that is also fine with me :) |
|
@Rahban1 Yea, that hashing seems fine! If I am not mistaken, it will hash the MarkdownAST's |
|
Yes @mortenpi you're right, it converts MarkdownAST node into a string representation and then hash it and uses the first 8 characters. I will implement it tomorrow since I am traveling today :) |
|
works fine for me locally. |
|
Sorry for the delay, but LGTM! Thank you @Rahban1! |
|
Thank you 😁 |
| @@ -2393,16 +2393,24 @@ function domify(dctx::DCtx, node::Node, a::MarkdownAST.Admonition) | |||
| # apply a class | |||
| isempty(cat_sanitized) ? "" : ".is-category-$(cat_sanitized)" | |||
| end | |||
|
|
|||
| node_repr = sprint(io -> show(io, node)) | |||
| content_hash = bytes2hex(SHA.sha1(node_repr))[1:8] | |||
There was a problem hiding this comment.
Unfortunately this has introduced a severe performance regressions. For the Oscar.jl package, building its manual went from ~40 seconds to ~680 seconds on my machine. It spends all that time computing SHA1 checksums it then doesn't need...
There was a problem hiding this comment.
Rats. Do you have any sense of whether it's the SHA calculation that's slow, or the rendering of the block (i.e. the sprint line)?
There was a problem hiding this comment.
I've interrupted this via ctrl-c a few (~5?) times and each time it was in the middle of a SHA computation. Not a proof yet but quite suggestive
There was a problem hiding this comment.
We don't need crypto safety here. Perhaps a weaker but faster hash suffices. Maybe even CRC32?
There was a problem hiding this comment.
wow, that's interesting. When working on this issue, I was testing this locally and I had realized a slight delay in build time like 5-10% increase so I thought it wouldn't be a big deal (maybe because Julia docs doesn't use admonitions that much) but I didn't realize this would have such a exponential effect on other docs. Apologies!
There was a problem hiding this comment.
Isn't Base.hash of the string sufficient?
Close #2505
I have added a new field to HTMLContext
admonition_counterswhich stores counters for each admonition so each one gets a unique identifier. The main challenge was generating unique id for each admonition which is the meat of the pr, added some css for hover functionality of linkBefore :
After :
works well when tested locally