-
Notifications
You must be signed in to change notification settings - Fork 0
Image Upload suggestions #45
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
base: main
Are you sure you want to change the base?
Conversation
@@ -24,7 +25,41 @@ public class Tenant | |||
[ForeignKey(nameof(BannerImageId))] | |||
public Image? BannerImage { get; set; } | |||
|
|||
[Coalesce] | |||
public async Task<ItemResult<Image>> UploadImageFile(AppDbContext db, [Inject] ImageService imageService, File file) |
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.
My code is verbose because I wrote this quickly, but the code given here can be reduced if you just make one shared function and 2 lambda functions. 🤷♀️
|
||
namespace IntelliTect.SyncUp.Data.Services; | ||
[Coalesce, Service] |
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.
Removed the coalesce service attribute because this never actually linked an image up with a tenant ( and it probably shouldn't, because we want to use this for groups, as well, and likely other models like comments... posts... etc)
This should just be an internal, non coalesce, service
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.
Not sure I fully undertstand...
@@ -6,7 +6,8 @@ | |||
accept="image/*" | |||
style="display: none" | |||
/> | |||
<v-hover v-slot="{ isHovering, props }"> | |||
<v-skeleton-loader v-if="busy" type="image" width="300" /> |
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.
This is ugly and jumps the UI around, but the focus wasn't UI... I just wanted to have some visual indicator that the image was busy.
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.
There should have been an image placeholder, but the skeleton might be a better idea.
|
||
const image = defineModel<Image | null>({ default: null }); | ||
|
||
const props = defineProps<{ | ||
cover?: boolean; | ||
height?: number; | ||
width?: number; | ||
viewModel: TenantViewModel; //| GroupViewModel; // Uncomment when implemented on the Group Model as well. |
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.
Important comment here, we can reuse this component for the GroupViewModel and other view models that work this way... Could also consider making this more strict with an interface on the backend.
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.
Why wouldn't we just pass in the image, then we can use it anywhere?
No description provided.