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

feat: link previews #305

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

sameoldlab
Copy link
Contributor

Adds link previews to articles. (closes #241)

image

@sameoldlab sameoldlab marked this pull request as ready for review February 7, 2025 16:08
@erlend-sh erlend-sh requested a review from zicklag February 10, 2025 14:25
Copy link
Collaborator

@zicklag zicklag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good!

I've got one question below.

Comment on lines 24 to 29
$effect(() => {
data.page.html;
untrack(() => {
previewLinks(data.page.html).then((res) => (html = res));
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this effect necessary? If the HTML changes it should be when we go to another page and in that case the preview links will already have been rendered by the server right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be taken out. I noticed some delay when loading pages with a lot of link previews (largest page I tried had 10 links, but was also from my local connection to the link service), so the effect was there for a second attempt to resolve any links that timed out server-side.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hopes that the link service's caching should prevent issues after the first load, lets take it out for now and see how it does on the main site.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Link previews
2 participants