Add anchor links to headings

srid / emanote

Emanate a structured view of your plain-text notes

https://emanote.srid.ca

Other

808 stars 71 forks source link

Add anchor links to headings #500

Closed TristanCacqueray closed 8 months ago

TristanCacqueray commented 8 months ago

Fixes #480

Uses https://github.com/srid/heist-extra/pull/6

TristanCacqueray commented 8 months ago

This is pulling #503 to make the group visibility on hover works (that seems broken with v2 ...). I did my best to make it look pretty, but that could use some polish, for example to move the anchor to the side... Well I think this is now ready for review.

TristanCacqueray commented 8 months ago

Actually there is an issue with the <base href=...> head element which makes <a href=#id> points at /#id instead of /page#id. It seems like this was introduced in 936b62d , and I guess it would be better to make relative links use the siteUrl instead of forcing / on every page. Otherwise we can make the anchor link absolute, but that seems wrong.

srid commented 8 months ago

@TristanCacqueray Great, this looks good to me. I'll do some final tweaks before merging. In particular I want to see if I can factor out the duplication.

srid commented 8 months ago

Actually there is an issue with the <base href=...> head element which makes <a href=#id> points at /#id instead of /page#id. It seems like this was introduced in 936b62d , and I guess it would be better to make relative links use the siteUrl instead of forcing / on every page. Otherwise we can make the anchor link absolute, but that seems wrong.

https://stackoverflow.com/questions/8108836/make-anchor-links-refer-to-the-current-page-when-using-base

Though the stackoverflow answer gives rise to glitchy scrolling.

kukimik commented 8 months ago

Actually there is an issue with the <base href=...> head element which makes <a href=#id> points at /#id instead of /page#id. It seems like this was introduced in 936b62d , and I guess it would be better to make relative links use the siteUrl instead of forcing / on every page. Otherwise we can make the anchor link absolute, but that seems wrong.

FWIW, absolute links were used in #361 to replace JavaScript for footnotes and I also planned to use them for headings, but never got to it. @TristanCacqueray Thanks for taking this up!

TristanCacqueray commented 8 months ago

@kukimik you are welcome, thank you folks for following up on that :)

It seems a bit janky with the live server, clicking on an anchor sometime scrolls at the bottom. I suspect the ema-shim.js is somehow selecting an outdated element and I wonder if waiting for a morphdom event before scrolling into view would help.

Also this still needs:

[ ] heist-extra version bump
[ ] breaking template changelog

srid commented 8 months ago

I suspect the ema-shim.js is somehow selecting an outdated element and I wonder if waiting for a morphdom event before scrolling into view would help.

Probably this. It would be great to fix this in Ema. Happy to accept a PR.

TristanCacqueray commented 8 months ago

Thank you @srid for the fixing the code and merging it quickly, that's great!