Link bubble menu to preview and edit links by mejo- · Pull Request #5158 · nextcloud/text

mejo- · 2023-12-23T23:51:49Z

📝 Summary

Opens a link bubble when clicking on a link and removes in-document link previews.

Opens link bubble when clicking on a link or navigating inside a link mark.
Displays a link preview in the bubble. Loads a fallback if no preview can be loaded.
Actions inside the bubble: copy link to clipboard, edit link URL, remove link mark.
Anchor links (href starting with #) don't open a link bubble but navigate to the anchor directly.
No longer display link previews in the document.
Ctrl + click and middle click on a link doesn't open link bubble. Instead opens the link directly in target="_blank".
Esc closes the link bubble.
Remove the possibility to customize link click handlers. Instead, we use the reference widgets now, and they should provide their own click handlers if desired.
Resolves: When editing documents, clicking links will open a new Markdown editor collectives#845
Resolves: Middle click on links must not insert text #2198
Resolves: Links in Text files will open 2x #3691

🖼️ Screenshots

Overview

Copy link to clipboard	Edit link	Remove mark	Edit mode

Screencast

🚧 TODO

🏁 Checklist

Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
Sign-off message is added to all commits
Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
Documentation is not required

juliusknorr · 2023-12-28T07:57:16Z

Looks very nice already. I stumbled over the following two issues that would be worth to check and address in the same run:

max-nextcloud

The overall approach looks good to me. I added some minor comments but nothing blocking.

src/components/Link/LinkBubbleView.vue

src/extensions/LinkBubblePluginView.js

src/marks/Link.js

max-nextcloud · 2024-01-29T13:44:24Z

Still much less of a drawback than not being able to append a word to the link name IMO.

I'm not sure why this is not working. I can append a word to an italic text fine - and from the tiptap perspective they are both marks.

update: looks like we inherit this behavior from both tiptap and prosemirror.

juliusknorr · 2024-01-29T13:57:38Z

The discussion about adding a description field should not block this PR imo, as it would be just another additional new feature. Maybe we can move that to an issue to discuss as a potential follow up.

juliusknorr · 2024-01-29T14:02:48Z

I'm not sure why this is not working. I can append a word to an italic text fine - and from the tiptap perspective they are both marks.
update: looks like we inherit this behavior from both tiptap and prosemirror.

Just for reference, quoting prosemirror docs:

By default, marks are inclusive, meaning that they get applied to content inserted at their end (as well as at their start when they start at the start of their parent node). For link-type marks, this is usually not the expected behavior, and the inclusive property on the mark spec can be set to false to disable that behavior.

And tiptap seems to auto set that if autolink is enabeld
https://github.com/ueberdosis/tiptap/blob/main/packages/extension-link/src/link.ts#L88

nimishavijay · 2024-01-29T16:03:19Z

Agreed with both that the link text can be edited in the popup, and that it can be a follow up PR :) This PR looks really great! Only some minor details for feedback:

Indeed the image preview can be smaller, as at this size the text will likely be ellipsized most of the time. How about 64px? :)
There can also be more space between the image preview and the edges of the popup, so some margin can be added
The border radius of the image preview should be nicely aligned with border radius of the popup, currently it's too less
When you are editing the link, the right and left spacing seems pretty tight. I suggest some left and right margin be added to the text field
In the screenshots there is no text on the left of the buttons, but in the screencast it's there. Is that intentional or am I missing something? I suggest that there doesn't need to be text, but instead there can be an "Open link" secondary button (what do you think @marcoambrosini ?) :)

mejo- · 2024-01-29T18:22:47Z

The failing Cypress test in workspaces.spec.js is because in CI we have http://localhost:8080 or something alike as local URL and NcReferenceList doesn't regard this as a valid URL. The following PR aims to change this: nextcloud-libraries/nextcloud-vue#5176

mejo- · 2024-01-30T11:41:38Z

Thanks for your feedback @marcoambrosini and @nimishavijay.

Indeed the image preview can be smaller, as at this size the text will likely be ellipsized most of the time. How about 64px? :)

Given that this would need to happen in the NcReferenceWidget component in nextcloud-vue, I'd suggest to make this a follow-up topic.

In the screenshots there is no text on the left of the buttons, but in the screencast it's there. Is that intentional or am I missing something? I suggest that there doesn't need to be text, but instead there can be an "Open link" secondary button (what do you think @marcoambrosini ?) :)

Addin an "open link" button is a bit more technically involved. The link preview from NcReferenceWidget implementations might have custom link handling implemented. Like opening files in the viewer, or opening other pages of a collective via re-routing in vue. Currently there is no easy way to "copy" this behaviour outside the widgets. So I'd rather move this to a follow-up as well.