Frontend: Proof of concept for image preview #2266
konrad
Loading…
x
Reference in New Issue
Block a user
No description provided.
Delete Branch "Elscrux/vikunja:feature/image-preview"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Adds a proof of concept preview for all images inside the task itself (not related to the cover image feature). Basically it is very convenient to show all images in a task when you click on it, rather than having to individually open images when you search for a specific one or if you want to get a quick overview.
This implementation shows how it could work, but is probably not the best way to do it. Please let me know what you think!
Hi Elscrux!
Thank you for creating a PR!
I've deployed the frontend changes of this PR on a preview environment under this URL: https://2266-feature-image-preview--vikunja-frontend-preview.netlify.app
You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somewhere. The easiest to use is https://try.vikunja.io/.
This preview does not contain any changes made to the api, only the frontend.
Have a nice day!
Needs some styling, but I like the idea!
@ -67,6 +67,12 @@{{ a.file.mime }}</span></p><imgI would extract this out into its own component, which takes the attachment, fetches the image and shows it next to the attachment. We could extend this further to show some kind of icon for pdfs or other file types later on.
Good point, I did that
It also seems like the preview is not shown when the file is initially uploaded.
4da881aa99tobe50d3d7b3WIP: Frontend: Proof of concept for image previewto Frontend: Proof of concept for image preview@konrad
This should be fixed now.
I styled it in a way so it's displaying like a table. I'm not super used to doing styling so please feel free to touch this up :)
@ -53,3 +30,1 @@><span v-tooltip="formatDateLong(a.created)">{{ formatDateSince(a.created) }}<table class="table table-striped">Please do not use a table for multi-column layout.
I used a grid instead now
@ -0,0 +1,34 @@<template><imgCan you make this always the same size? Use
object-fit: cover;to let the image fill the container nicely.I included it, but it didn't seem like it made a difference
Did you set a fixed with and height for the image?
Yes, it's at 4rem now
@ -0,0 +29,4 @@<style scoped lang="scss">img {border-radius: 0.5rem;Please use
$radiushere so that the radius is consistent with other elements.Done!
@ -436,2 +447,4 @@}.preview-column {max-width: 125px;Please use
remor an existing scss variable (same for the other occurences)Done
0c201cba59to6fa15d30ad@ -0,0 +2,4 @@<img:src="blobUrl"alt="Attachment preview"style="object-fit: cover;"Please do not use inline styles.
This image should have a rectangular fixed size, then the
object-fitproperty will make sure it looks good.Please take another look at the failing CI.
@ -34,3 +31,3 @@v-for="a in attachments":key="a.id"class="attachment"class="grid-item clickable"grid-itemsounds like a child of a grid. Why not call itattachmentas it was before?@ -437,0 +453,4 @@max-height: 4rem;}.clickable {What happens when the item is clicked?
The image preview is shown, like previously. Nothing changed in that regard. Previously, an tag was used to have the cursor change to a clickable cursor. This comment was there and I fixed it by adding this clickable class.
In terms of a11y, if it's actually clickable, it should be a button or a. Divs are usually not clickable.
Fair. I changed this and it has a little different feel to it now. Let me know what you think!
Looks good! Will do a proper review when I'm back at my computer, early next week.
Here's what I meant with the
widthchange:However, it should keep the spaces between the actions and not add left spacing to the filename.
@ -43,3 +43,3 @@cy.wait('@uploadAttachment')cy.get('.attachments .attachments .files a.attachment')cy.get('.attachments .attachments .files div.attachment')This needs to be changed again to make the test pass.
@ -0,0 +30,4 @@<style scoped lang="scss">img {border-radius: $radius;object-fit: cover;Adding a
width: 100%here will make it look like what I meant (will share a screenshot later)When uploading a file which is not previewable (like a pdf), it still shows an empty box:
(it also shows the "Make cover" option, but that's a problem for another day)
Imo this is intended behavior. Otherwise you would have an inconsistent spacing to the left border between different file types attachments. The idea is that everything is everything is in one column.
What could be done here is have the preview width be based on the the maximum preview width and capped to a value, so that there would be no preview field when only a pdf without preview is attached. That is why I used a table in the first place.
I also think not having it spaced out would feel weird, but now there is a big empty space which feels even worse. Can you add a file icon as a placeholder? We can add other icons or preview images for more file types later on.
The attachment title should not have spacing to the left, it should be aligned with the other information below.
@konrad I've added a series of changes, let me know what you think, especially about the icons for the attachment actions
Pretty good already!
The spacing on this card should be improved so that
@ -105,0 +94,4 @@><icon icon="trash-alt" /></BaseButton><BaseButtonThis needs a tooltip
@ -0,0 +7,4 @@<iconv-elsesize="6x"icon="file-image"Can you use a simple file icon here? An image icon suggests the file is an image, which it is not.
Also please make the file icon in a dark gray, similar to the attachment title.
@konrad all done, let me know if this works. I wasn't quite able to have the filename align to the top of the image because the cover image label is bigger that the filename text and would stick out when I tried that. I've aligned everything with equal spacing now though.
Love it! One tiny little thing and then we're good to go.
@ -298,11 +307,17 @@ async function setCoverImage(attachment: IAttachment | null) {}.attachment {Please make this
width: 100%.done!
Thanks!