text icon indicating copy to clipboard operation
text copied to clipboard

Link preview widgets

Open juliusknorr opened this issue 2 years ago • 9 comments

Implement link preview widgets for paragraphs that only contain a link.

Screenshot 2022-09-21 at 15 55 12

juliusknorr avatar Sep 21 '22 13:09 juliusknorr

Whoohoo, super nice :heart_eyes: Works like a charm.

I wonder though, whether there should be an option to remove the preview widget. Imagine e.g. a list of links. Legibility would decrease with the huge preview widgets in between each item in my eyes.

mejo- avatar Sep 21 '22 14:09 mejo-

Another frontend comment: I think it would look better if we displayed either the link or the widget. Now the link is always duplicated. Once the typed text and a second time at the bottom of the widget.

Even though, thinking about it, no idea how the link could be updated if only the widget is displayed.

mejo- avatar Sep 21 '22 14:09 mejo-

I get 404 responses for open-graph link previews (/core/references/preview/3097fca9b1ec8942c4305e550ef1b50a)

Do you have memcache enabled?

The RenderReferenceEvent is not emitted by Text so the reference frontend stuff is not loaded with Text so we get Widget for rich object type integration_github not registered for every provider.

Ah good point, will add that.

juliusknorr avatar Sep 21 '22 14:09 juliusknorr

Another frontend comment: I think it would look better if we displayed either the link or the widget. Now the link is always duplicated. Once the typed text and a second time at the bottom of the widget.

Even though, thinking about it, no idea how the link could be updated if only the widget is displayed.

Yes, updating was the main reason to always show it. About the option to hide it, I'd agree that it would be nice to have, but we cannot really persist it in markdown, so it would need a per user setting that we store or something like this.

juliusknorr avatar Sep 21 '22 14:09 juliusknorr

Design feedback moved to review :)

jancborchardt avatar Sep 21 '22 14:09 jancborchardt

I get 404 responses for open-graph link previews (/core/references/preview/3097fca9b1ec8942c4305e550ef1b50a)

Do you have memcache enabled?

I realized right after writing my comment that i had commented the caching stuff in the ReferenceManager. All good now. Sorry for the noise.

The RenderReferenceEvent is not emitted by Text so the reference frontend stuff is not loaded with Text so we get Widget for rich object type integration_github not registered for every provider.

Ah good point, will add that.

I tried dispatching the RenderReferenceEvent event in lib/Listeners/LoadViewerListener.php right after loading the 'text-viewer' script and it works fine.

julien-nc avatar Sep 21 '22 14:09 julien-nc

@mejo- agree, we discussed before there should ideally be a "close x" on the top right to hide the preview, but decided it's something for a future iteration.

And we also talked about showing both vs only showing the preview, and came to the same conclusion as you with the editing issue.

In a future version, we could make it so that only the preview is shown, and it has a 3-dot menu for "Edit link" and "Hide preview"

jancborchardt avatar Sep 21 '22 14:09 jancborchardt

About the option to hide it, I'd agree that it would be nice to have, but we cannot really persist it in markdown, so it would need a per user setting that we store or something like this.

Yeah, I see the point. I wondered whether it would be an option to add the preview as a separate node to markdown. There's e.g. junkawa/markdown-it-link-preview. The preview would be auto-generated when inserting an URL, but once it got removed manually it would stay away persistently in the document. What do you think about it?

And we also talked about showing both vs only showing the preview, and came to the same conclusion as you with the editing issue. In a future version, we could make it so that only the preview is shown, and it has a 3-dot menu for "Edit link" and "Hide preview"

That's a great idea!

mejo- avatar Sep 22 '22 07:09 mejo-

/backport to stable25

juliusknorr avatar Sep 22 '22 10:09 juliusknorr

I could not reproduce the failures reliably locally and they seem to be unrelated, so I'd consider failing cypress tests (outside of the link related ones) as not blocking for the merge.

juliusknorr avatar Oct 10 '22 17:10 juliusknorr

/compile

juliusknorr avatar Oct 12 '22 12:10 juliusknorr

The backport to stable25 failed. Please do this backport manually.