-
Notifications
You must be signed in to change notification settings - Fork 94
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
Link preview widgets #2977
Link preview widgets #2977
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I get 404 responses for open-graph link previews (/core/references/preview/3097fca9b1ec8942c4305e550ef1b50a)
- 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.
Whoohoo, super nice 😍 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. |
}, | ||
methods: { | ||
getTextReference(text) { | ||
const PATTERN = /(^)(https?:\/\/)?((?:[-A-Z0-9+_]+\.)+[-A-Z]+(?:\/[-A-Z0-9+&@#%?=~_|!:,.;()]*)*)($)/ig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can move it to outside of component methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me export that from the @nextcloud/vue-richtext
one separately
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. |
Do you have memcache enabled?
Ah good point, will add that. |
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. |
Design feedback moved to review :) |
This comment was marked as resolved.
This comment was marked as resolved.
@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" |
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?
That's a great idea! |
/backport to stable25 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff! :) Design feedback:
- There seems to be some slight left margin of the container, making it not fully left-aligned with the text
- The container has the same mrgin to top and bottom, making it unclear which link it belongs to. It should have a bit less margin topl the top and a bit more to the bottom. (E.g. --default-grid-baseline on top, and *3 for bottom. – or *2 top and *4 bottom, whatever looks better.)
- The container has more padding below the last line of text (link) than above the title. Maybe we need to reduce the image size a bit to fix this?
Possibly these have to be fixed in the component instead?
e8d694c
to
43f1433
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woop woop
986b1db
to
3387303
Compare
18cebfa
to
b5696b2
Compare
b5696b2
to
bbec680
Compare
66964dd
to
fde3e3f
Compare
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. |
46e853a
to
3ceb1bf
Compare
Signed-off-by: Julius Härtl <jus@bitgrid.net> Address review feedback Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
3ceb1bf
to
ad729fc
Compare
/compile |
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
The backport to stable25 failed. Please do this backport manually. |
Implement link preview widgets for paragraphs that only contain a link.