Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for resizable image inputs by introducing a new ImageSize type with four size variants ("small", "medium", "large", "wide") and updating the image upload component to accept and apply size-specific styling.
Key changes:
- Added
ImageSizetype and integrated it into the abstract resource form input system - Updated
ImageUploadcomponent to support size prop with responsive CSS classes - Applied appropriate size specifications to image fields in resource metadata
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/features/abstract-resource-form/types/inputs.ts |
Defines new ImageSize type and adds optional size property to image input configuration |
src/features/abstract-resource-form/index.ts |
Exports ImageSize type for use throughout the application |
src/components/inputs/image-upload.tsx |
Implements size prop with responsive styling through getImageSizeClasses function and updated InputBox component |
src/features/resources/data/resource-metadata.ts |
Applies size specifications to banner and logo image fields (wide for banners, small for logos) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| className="aspect-video h-fit max-h-48 w-full overflow-hidden rounded-lg md:size-48" | ||
| className={cn( | ||
| "h-fit w-full overflow-hidden rounded-lg", | ||
| size !== "wide" && "aspect-video max-h-48", |
There was a problem hiding this comment.
There's a potential CSS conflict for small/medium/large sizes. The aspect-video class (line 55) applies at all breakpoints, while size-specific classes like md:size-32 (line 56) set explicit square dimensions at md+ breakpoint. This creates competing constraints where aspect-video requests a 16:9 ratio but md:size-32 sets equal width and height. Consider applying aspect-video only below the md breakpoint by using max-md:aspect-video, or clarify if the current behavior where explicit dimensions override aspect ratio is intentional.
| size !== "wide" && "aspect-video max-h-48", | |
| size !== "wide" && "max-md:aspect-video max-h-48", |
28ea9c7 to
5742571
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return "md:size-64"; | ||
| } | ||
| case "wide": { | ||
| return "md:h-48 md:w-full aspect-video"; |
There was a problem hiding this comment.
The "wide" size applies md:h-48, md:w-full, and aspect-video simultaneously. This creates a conflict: md:w-full makes the element full width, aspect-video wants to maintain a 16:9 ratio, but md:h-48 fixes the height at 48px. These constraints cannot all be satisfied simultaneously. Consider either removing md:h-48 to let the aspect ratio control the height based on the full width, or replacing aspect-video with a max-height constraint if a fixed height is desired.
| return "md:h-48 md:w-full aspect-video"; | |
| return "md:w-full aspect-video"; |
5742571 to
1250677
Compare
kguzek
left a comment
There was a problem hiding this comment.
w sumie wydaje mi się że ta cała implementacja z rozmiarami jest zbędna. lepiej będzie zrobić żeby wszystkie zdjęcia typu ImageType.Banner wyświetlały się na górze formularza nad wszystkimi pozostałymi inputami, a tylko Logo żeby były z boku na desktopie. dodanie dodatkowego atrybutu size tylko namiesza, a to nie warto bo mamy zdjęcia dosłownie chyba tylko w trzech formularzach
| function getImageSizeClasses(size: ImageSize): string { | ||
| switch (size) { | ||
| case "small": { | ||
| return "md:size-32"; | ||
| } | ||
| case "medium": { | ||
| return "md:size-48"; | ||
| } | ||
| case "large": { | ||
| return "md:size-64"; | ||
| } | ||
| case "wide": { | ||
| return "md:h-48 md:w-full aspect-video"; | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
to nie powinna być funkcja ze switchem tylko obiekt jako mapa
| className="aspect-video h-fit max-h-48 w-full overflow-hidden rounded-lg md:size-48" | ||
| className={cn( | ||
| "h-fit w-full overflow-hidden rounded-lg", | ||
| size !== "wide" && "aspect-video max-h-48", |
There was a problem hiding this comment.
użyj notacji { "aspect-vide max-h-48": size !== "wide" } zamiast && dla spójności z resztą kodu
| value, | ||
| label, | ||
| type = ImageType.Logo, | ||
| size = "medium", |
There was a problem hiding this comment.
to zły pomysł żeby to były string literały. zmień to na enuma
1250677 to
28a3a12
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| case "wide": { | ||
| return "md:h-48 md:w-full aspect-video"; | ||
| } |
There was a problem hiding this comment.
getImageSizeClasses is declared to return string, but with no default/exhaustive guard the function can return undefined at runtime if an unexpected size value slips through (or if the ImageSize union is extended later). Consider adding an explicit fallback return, or an exhaustive never check to make this function total.
| } | |
| } | |
| default: { | |
| const _exhaustiveCheck: never = size; | |
| throw new Error(`Unhandled image size: ${_exhaustiveCheck}`); | |
| } |
|
|
||
| export * from "./providers/arf-sheet-provider"; | ||
|
|
||
| export type { ImageSize } from "./types/inputs"; |
There was a problem hiding this comment.
ImageSize is being re-exported from the feature root, but most consumers import ARF types from @/features/abstract-resource-form/types (e.g. src/features/resources/types/internal.ts:6). To keep the public type surface consistent, consider also re-exporting ImageSize from src/features/abstract-resource-form/types/index.ts (or switching call sites to import from the same barrel).
| export type { ImageSize } from "./types/inputs"; |

Resolves #89
This pull request adds support for specifying image size variants in the image upload input component and updates resource metadata to use these new size options. The changes improve flexibility and consistency in how images are displayed throughout the application.
Image upload component enhancements:
ImageSizetype with options"small","medium","large", and"wide", and updated theImageUploadcomponent to accept asizeprop for controlling image display size. [1] [2] [3] [4] [5]InputBoxcomponent to apply different CSS classes based on the selected image size, ensuring images are rendered appropriately for their context.InputBoxwithinImageUploadto pass the newsizeprop. [1] [2]Resource form and metadata updates:
sizeproperty for image inputs.RESOURCE_METADATAto specify appropriate image sizes for various image fields, such as using"wide"for banners and"small"for logos. [1] [2] [3]Type and export adjustments:
ImageSizetype from the abstract resource form module for use in other parts of the codebase.