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

Add support for bookmark thumbnails #721

Merged
merged 13 commits into from
May 7, 2024

Conversation

vslinko
Copy link
Contributor

@vslinko vslinko commented May 2, 2024

Hello!

I want to add support for bookmark thumbnails. I've made a rough implementation, but I'm not sure I'm going in the right direction. please tell me what else I should consider or change in the code.

todo:

  • check and fix tests
  • add more tests
  • handle relative path in og:image
  • download previews and serve it from local dir
  • show preview image in edit form
  • show preview image in details popup
  • squash commits and write good commit message

@sissbruecker
Copy link
Owner

Thanks for your effort, looks promising but I'll have to take some time to think about how this should work. Some initial thoughts:

  • Having two fields for storing the preview image (url or downloaded image) feels a bit weird. I can see the benefit though, as that allows showing the preview image right in the form after loading website metadata. And downloading the image later helps in case the website goes offline. Though for a start, maybe just storing the URL would be enough, would also make the PR a bit smaller and easier to review.
  • Not sure if the image really needs to show up in the create / edit form. It's kind of neat, but also takes up a lot of space.
  • The bookmark list is kind of hard to read at the moment. Having the images at the start of each row makes it harder to distinguish individual bookmarks in case some of them are missing a preview image. It's also very noisy - preview and favicon are next to each other, preview images have different sizes. I think it would be better to put the images at the end of each line, give them a fixed size, and make each image cover the full size. Quick prototype below. It also takes up a lot of space on mobile, I would consider hiding the preview there.
  • There needs to be some way to load preview images for existing bookmarks, similar to the refresh favicons button. Makes sense to do this in a separate PR though.

I'll try to take a closer look soon, no need to do any further changes to the PR for now. I might also decide to make some changes directly myself if it turns out to be low effort.


Bildschirmfoto 2024-05-05 um 20 39 55

@vslinko
Copy link
Contributor Author

vslinko commented May 6, 2024

Having two fields for storing the preview image (url or downloaded image) feels a bit weird. I can see the benefit though, as that allows showing the preview image right in the form after loading website metadata. And downloading the image later helps in case the website goes offline. Though for a start, maybe just storing the URL would be enough, would also make the PR a bit smaller and easier to review.

I started with one column, replacing the remote URL with the local URL after downloading. But to do that you have to do schema checks for the URL in many places in the code, which didn't seem as convenient as keeping two links. But yeah, you could keep one field.

But anyways I see it as important to be able to store images locally, as it has many advantages:

  • external resources won't know about every time I visit my bookmarks;
  • external sites may respond slowly, making my UX worse;
  • external sites die regularly.

Perhaps you can make two settings: enabling the preview viewing feature, and enabling the preview download feature, which are independent of each other.

Not sure if the image really needs to show up in the create / edit form. It's kind of neat, but also takes up a lot of space.

Agree

The bookmark list is kind of hard to read at the moment. Having the images at the start of each row makes it harder to distinguish individual bookmarks in case some of them are missing a preview image. It's also very noisy - preview and favicon are next to each other, preview images have different sizes. I think it would be better to put the images at the end of each line, give them a fixed size, and make each image cover the full size. Quick prototype below. It also takes up a lot of space on mobile, I would consider hiding the preview there.

Agree

There needs to be some way to load preview images for existing bookmarks, similar to the refresh favicons button. Makes sense to do this in a separate PR though.

Agree :)

@vslinko
Copy link
Contributor Author

vslinko commented May 6, 2024

I would consider hiding the preview there

I think that if a person enables previews, they should work on mobiles. Or at least it should be possible to configure it.

My use case implies that I want to see previews on mobile, because I keep many YouTube links and by previews I can read faster what kind of content it is than by description.

@sissbruecker
Copy link
Owner

To make it a bit simpler I've removed the model field for storing the URL and only kept the field for storing the file. With that it works the same way as the favicons. That means it might take a while until the preview image shows up, basically until the respective task has been run. But that should be fine for regular usage.

Styles could maybe use some tweaking here and there, but should be good enough for a start. Long term it would be good to have an option for showing separators in the bookmark list, I think that would really help with distinguishing the bookmarks and their images from each other. For now some custom CSS could be added as a workaround.

Thanks again for the contribution, apart from some minor tweaks it was pretty complete. Note that it might take me a few weeks to release this, I currently don't have access to the machine where I cross compile the Docker images.

@sissbruecker sissbruecker changed the title Preview Image Add support for bookmark thumbnails May 7, 2024
@sissbruecker sissbruecker merged commit 87cd406 into sissbruecker:master May 7, 2024
2 checks passed
@vslinko vslinko deleted the preview_image branch May 7, 2024 18:59
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.

2 participants