Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Add page anchors and inter page links #2464

Closed
wants to merge 5 commits into from
Closed

Feature: Add page anchors and inter page links #2464

wants to merge 5 commits into from

Conversation

susnux
Copy link
Contributor

@susnux susnux commented May 30, 2022

Summary

Implements the anchors needed for inter-page links

This requests consists of three commits, the first part adds the required anchors.
IDs are generated using a similar slugify function github and
gitlab use.
For initial content markdown-it-anchor is used.

The second part implements visible links, using ProseMirror view decorations.

The last part is handling opening inter-page links, as just adding an onClick listener
using Element.scrollIntoView() would not handle user defined links within the document.
And the current default handler would open the document again in a new tab.

susnux added 3 commits May 30, 2022 21:25
Implements the anchors needed for inter-page links, closes #2173.
IDs are generated using a similar slugify function github and
gitlab use.
For initial content markdownit-anchor is used.

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
Using ProseMirror view decorations to display visible
anchor links on headings.

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@susnux susnux changed the title Feature/inter page Feature: Add page anchors and inter page links May 30, 2022
@juliusknorr juliusknorr added 3. to review enhancement New feature or request labels May 31, 2022
@juliusknorr juliusknorr added this to the Nextcloud 25 milestone May 31, 2022
@vinicius73 vinicius73 self-assigned this Jun 1, 2022
@vinicius73
Copy link
Member

/compile amend/

@vinicius73
Copy link
Member

I've just taken a fast look into this implementation and remove this behavior from workspace rich editor.

Despite small CSS bugs (master branch) seem it works.
But I must check it close, decorators looks a heavy approach.

It is the only way to do it? @susnux

@susnux
Copy link
Contributor Author

susnux commented Jun 1, 2022

@vinicius73

Despite small CSS bugs (master branch) seem it works.

Sorry, CSS is not one of my strengths, I get it work but it might not that clean (help one the CSS part is highly welcome)

But I must check it close, decorators looks a heavy approach.

I thought it might be the easiest way, as the links should only be visible on the frontend and not serialized.
Using decorations will not interfere with the editor state and so not interfere with any collaboration stuff.
It should no lead to performance losses, as the decorations are cached and only re-rendered if the ID changed (user changed the heading or removed a heading with the same content and type above) or the position changed and the decoration has to be rendered new anyways.

It is the only way to do it? @susnux

No, two other option I can imagine:

  • Using a NodeView for headings
    • pro: No Decorations (this would be the tiptap way) and still no unneeded nodes in the editor state
    • con: Requires to override the renderHTML of the Heading class, so it might get hard to inherite stuff from the core heading (rewrite of existing code)
  • Insert a custom AnchorLink Node into the editor state, the same way the IDs are updated
    • pro: At least no decorations needed
    • con: Part of the editor state, as such they are handled by transactions (collab?) and need to serialized (or dropped)

So from my point decorations are a cleaner way, but NodeView could be also a option.

@susnux
Copy link
Contributor Author

susnux commented Jun 1, 2022

The failing tests are somewhat related to #2217 as they come from the Commonmark test, but this PR adds the ID attribute to headings.
Not sure if the commonmark test is required at all, as it only tests markdown-it without any ProseMirror / Tiptap interaction. And markdown-it is tested by upstream anyway.

@juliusknorr
Copy link
Member

Extending the Heading node sounds reasonable to me. We already extend from the tiptap one in

const Heading = TipTapHeading.extend({
so that should give a starter point for adjusting the renderHTML. It may only require the onUpdate event https://tiptap.dev/guide/custom-extensions#events in case the anchor should be updated on a content change.

@susnux
Copy link
Contributor Author

susnux commented Jun 6, 2022

@vinicius73 @juliushaertl How about doing it this way: master...susnux:feature/lightweight-inter-page

I rewrote it to be similar to the already existing custom nodes.

@juliusknorr
Copy link
Member

Yes, looks reasonable to me from a first look. @vinicius73 What do you think?

@susnux
Copy link
Contributor Author

susnux commented Jun 17, 2022

created a new PR from the modified (lightweight) version: #2520

@susnux susnux closed this Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature-request: inter-page links
3 participants