Feat/image-upload-component #3654
Conversation
📝 WalkthroughWalkthroughAdds a new ImageUpload feature (dropzone, canvas cropper, controllers, utils, hook, config, summaries, tests), updates Dropzone API and FileUpload integration, adjusts Card/Cards styling and AppCard API, and adds i18n keys plus E2E helpers. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Magnusrm
added
kind/product-feature
…tsimplify mouse dragging and support touch dragging (#3656)
* reuse dropzonecomponent and display only when imgsrc is not defined * bring back the reset button * move controller section to its own file
* move image cropper into card wip * working cropping component in card * conforrm to styling naming in repo * canvas height adjustment --------- Co-authored-by: lassopicasso <[email protected]>
* clean up css and style closer to figma design
…fn) (#3668) * added save and cancel buttons with functionality (some left for save fn) * Adding AttachmentsPlugin to ImageUpload component (it needs to be defined as a form component for this to work, which is more fitting) * save cropped image to backend * feedback * small fix --------- Co-authored-by: Ole Martin Handeland <[email protected]>
* added save and cancel buttons with functionality (some left for save fn) * Adding AttachmentsPlugin to ImageUpload component (it needs to be defined as a form component for this to work, which is more fitting) * save cropped image to backend * redesign slider + buttons --------- Co-authored-by: Ole Martin Handeland <[email protected]>
* wip * revert unneccesary change * clean up
* add hook that handles the saved image and controllers buttons when saved
* support custom config * support mismatch of height and width if type is circle
* remove dropzone icon and refactor css * replace look of placeholder * remove unused css * remove focus for now
* cursor change to grabbing when active in canvas * make change image button support keyboard
* add validationmessages for imageupload size and types * clear validationmessages on cancel
* add language texts and use them throughout the component * change wording slightly and remove unused text * make file-endings the same
|
Not sure if I should do changes that SnoardCloud warns about. Have used hooks in the index file for the component, and I see that is done in other components as well. Regarding cypress, I have tested this locally and it works, but the tests fails here because of image upload isn't in the app that is tested in github actions. I can update it, but then other PR's will fail. Don't know what the best approach is. |
|
/publish |
PR release:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
src/layout/ImageUpload/imageUploadUtils.ts (4)
34-45: Use naturalWidth/naturalHeight and guard against zero.The function uses
image.width/image.height(CSS pixels) instead ofimage.naturalWidth/image.naturalHeight(actual image pixels), which can cause incorrect calculations. Also needs guards against zero dimensions to avoidInfinityorNaNin clamp calculations.Apply this diff to fix:
export function constrainToArea({ image, zoom, position, cropArea }: ConstrainToAreaParams): Position { - const scaledWidth = image.width * zoom; - const scaledHeight = image.height * zoom; + const iw = image.naturalWidth || image.width || 1; + const ih = image.naturalHeight || image.height || 1; + const scaledWidth = iw * zoom; + const scaledHeight = ih * zoom; const clampX = scaledWidth > cropArea.width ? (scaledWidth - cropArea.width) / 2 : 0; const clampY = scaledHeight > cropArea.height ? (scaledHeight - cropArea.height) / 2 : 0; const newX = Math.max(-clampX, Math.min(position.x, clampX)); const newY = Math.max(-clampY, Math.min(position.y, clampY)); return { x: newX, y: newY }; }Based on past review comments.
54-61: Use naturalWidth/naturalHeight for pixel-accurate placement.Same issue as
constrainToArea: should usenaturalWidth/naturalHeightwith zero guards for accurate canvas calculations.Apply this diff:
export const imagePlacement = ({ canvas, img, zoom, position }: ImagePlacementParams) => { - const scaledWidth = img.width * zoom; - const scaledHeight = img.height * zoom; + const iw = img.naturalWidth || img.width || 1; + const ih = img.naturalHeight || img.height || 1; + const scaledWidth = iw * zoom; + const scaledHeight = ih * zoom; const imgX = (canvas.width - scaledWidth) / 2 + position.x; const imgY = (canvas.height - scaledHeight) / 2 + position.y; return { imgX, imgY, scaledWidth, scaledHeight }; };Based on past review comments.
98-115: Guard zoom calculations against invalid inputs and clamp outputs.
Math.logwith non-positive values yields-InfinityorNaN. The functions lack guards forminZoom/maxZoom≤ 0 andvalue≤ 0 inlogToNormalZoom. Also, the normalized result should be clamped to [0, 100].Apply this diff:
function getLogValues({ minZoom, maxZoom }: ZoomParams): { logScale: number; logMin: number } { + if (minZoom <= 0 || maxZoom <= 0) { + throw new Error('minZoom and maxZoom must be > 0'); + } const logMin = Math.log(minZoom); const logMax = Math.log(maxZoom); return { logScale: (logMax - logMin) / 100, logMin }; } export function normalToLogZoom({ value, minZoom, maxZoom }: CalculateZoomParams): number { const { logScale, logMin } = getLogValues({ minZoom, maxZoom }); - return Math.exp(logMin + logScale * value); + const zoom = Math.exp(logMin + logScale * value); + return Math.min(maxZoom, Math.max(minZoom, zoom)); } export function logToNormalZoom({ value, minZoom, maxZoom }: CalculateZoomParams): number { const { logScale, logMin } = getLogValues({ minZoom, maxZoom }); if (logScale === 0) { return 0; } - return (Math.log(value) - logMin) / logScale; + const safeValue = Math.max(value, Number.EPSILON); + const normalized = (Math.log(safeValue) - logMin) / logScale; + return Math.min(100, Math.max(0, normalized)); }Based on past review comments.
118-119: Use naturalWidth/naturalHeight and guard against zero in calculateMinZoom.Same issue: should use
naturalWidth/naturalHeightand guard against zero dimensions to avoid division by zero orInfinity.Apply this diff:
-export const calculateMinZoom = ({ img, cropArea }: CalculateMinZoomParams) => - Math.max(cropArea.width / img.width, cropArea.height / img.height); +export const calculateMinZoom = ({ img, cropArea }: CalculateMinZoomParams) => { + const iw = img.naturalWidth || img.width || 1; + const ih = img.naturalHeight || img.height || 1; + return Math.max(cropArea.width / iw, cropArea.height / ih); +};Based on past review comments.
src/layout/ImageUpload/ImageDropzone.tsx (1)
19-26: Fix ARIA: move dragLabelId to labelId prop.
dragLabelIdshould be passed toDropzonevia thelabelIdprop (foraria-labelledby) rather than included indescribedBy(foraria-describedby). This ensures proper semantic ARIA relationships.Apply this diff:
- const ariaDescribedBy = [descriptionId, dragLabelId, formatLabelId].filter(Boolean).join(' '); + const ariaDescribedBy = [descriptionId, formatLabelId].filter(Boolean).join(' ') || undefined;And add
labelId={dragLabelId}to the Dropzone props (see next comment).Based on past review comments.
src/layout/ImageUpload/ImageCropper.tsx (3)
21-24: Add readOnly prop to support form state.The component lacks a
readOnlyprop, so the upload UI can't be disabled based on form state (e.g., when viewing submitted data).Apply this diff:
interface ImageCropperProps { baseComponentId: string; cropArea: CropArea; + readOnly?: boolean; }Then thread
readOnlythrough toImageDropzoneand disable interactions inImageCanvas/ImageControllerswhen true.Based on past review comments.
91-112: Fix memory leak: clean up Image and FileReader on unmount or new upload.The
ImageandFileReadercreated inhandleFileUploadare not cleaned up if the component unmounts or a new file is uploaded while loading. This causes memory leaks and potential stale callbacks.Apply this diff:
+ const loadingImageRef = useRef<HTMLImageElement | null>(null); + const readerRef = useRef<FileReader | null>(null); + const handleFileUpload = (file: File) => { const validationErrors = validateFile(file); setValidationErrors(validationErrors); if (validationErrors.length > 0) { return; } + // Clean up any previous loading + if (loadingImageRef.current) { + loadingImageRef.current.onload = null; + loadingImageRef.current.src = ''; + loadingImageRef.current = null; + } + if (readerRef.current) { + readerRef.current.abort(); + readerRef.current = null; + } + const reader = new FileReader(); + readerRef.current = reader; reader.onload = (event) => { const result = event.target?.result; if (typeof result === 'string') { const img = new Image(); + loadingImageRef.current = img; img.id = file.name; img.onload = () => { + if (loadingImageRef.current === img) { updateImageState({ minZoom: calculateMinZoom({ img, cropArea }), img }); + loadingImageRef.current = null; + } }; img.src = result; } }; reader.readAsDataURL(file); }; + + // Clean up on unmount + useEffect(() => { + return () => { + if (loadingImageRef.current) { + loadingImageRef.current.onload = null; + loadingImageRef.current.src = ''; + } + if (readerRef.current) { + readerRef.current.abort(); + } + }; + }, []);Based on past review comments.
114-143: Add error handling for toBlob failures.The
toBlobcallback only handles null blob with an early return but doesn't provide user feedback or logging when blob creation fails.Apply this diff:
cropCanvas.toBlob((blob) => { if (!blob) { + console.error('Failed to create image blob'); + setValidationErrors([{ key: 'image_upload_component.error_save_failed' }]); return; } const fileName = img.id; const imageFile = new File([blob], fileName, { type: 'image/png' }); saveImage(imageFile); setValidationErrors(null); }, 'image/png');Note: You'll need to add the error translation key to the language files.
Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/language/texts/en.ts(1 hunks)src/language/texts/nb.ts(1 hunks)src/language/texts/nn.ts(1 hunks)src/layout/ImageUpload/ImageControllers.tsx(1 hunks)src/layout/ImageUpload/ImageCropper.tsx(1 hunks)src/layout/ImageUpload/ImageDropzone.tsx(1 hunks)src/layout/ImageUpload/imageUploadUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/language/texts/nn.ts
- src/layout/ImageUpload/ImageControllers.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/language/texts/nb.tssrc/layout/ImageUpload/ImageDropzone.tsxsrc/layout/ImageUpload/imageUploadUtils.tssrc/language/texts/en.tssrc/layout/ImageUpload/ImageCropper.tsx
🧬 Code graph analysis (2)
src/layout/ImageUpload/ImageDropzone.tsx (4)
src/app-components/Dropzone/Dropzone.tsx (2)
IDropzoneProps(15-27)Dropzone(31-99)src/hooks/useDeviceWidths.ts (1)
useIsMobileOrTablet(33-35)src/components/label/Label.tsx (1)
getDescriptionId(135-141)src/features/language/Lang.tsx (1)
Lang(15-23)
src/layout/ImageUpload/ImageCropper.tsx (7)
src/layout/ImageUpload/imageUploadUtils.ts (9)
CropArea(8-8)Position(1-1)ErrorTypes(122-122)calculateMinZoom(118-119)constrainToArea(34-45)imagePlacement(54-61)validateFile(123-144)cropAreaPlacement(66-70)drawCropArea(79-87)src/layout/ImageUpload/useImageFile.tsx (1)
useImageFile(17-52)src/layout/ImageUpload/ImageDropzone.tsx (1)
ImageDropzone(19-59)src/app-components/Card/Card.tsx (1)
AppCard(20-54)src/layout/ImageUpload/ImageCanvas.tsx (1)
ImageCanvas(25-161)src/layout/ImageUpload/ImageControllers.tsx (1)
ImageControllers(24-148)src/features/language/Lang.tsx (1)
Lang(15-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Install
- GitHub Check: Analyze (javascript)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
🔇 Additional comments (5)
src/language/texts/nb.ts (1)
212-219: LGTM — Translation keys added correctly.The eight new Norwegian translation keys for the image upload component are well-formed and consistent with the existing translation structure.
src/language/texts/en.ts (1)
211-219: LGTM — English translations added correctly.The translation keys are clear and align with the Norwegian translations. The error messages and UI labels are user-friendly.
src/layout/ImageUpload/imageUploadUtils.ts (2)
10-25: LGTM — Crop area logic is correct.The function properly defaults to a 250×250 circle crop and enforces square dimensions for circular crops by using the minimum dimension.
121-144: LGTM — File validation logic is correct.The validation properly checks file size (10MB limit) and file extensions using case-insensitive suffix matching. Error handling is clear and user-friendly.
src/layout/ImageUpload/ImageCropper.tsx (1)
176-209: LGTM — AppCard usage is correct.The component properly renders
AppCardwithImageCanvasas media andImageControllersas children. The previous ref forwarding issue has been resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/layout/ImageUpload/imageUploadUtils.ts (1)
121-122: Consider exportingMAX_FILE_SIZE_MBfor better maintainability.The validation limit is hard-coded here but not exposed. If the limit changes, both this constant and any related translation strings must be updated. Exporting
MAX_FILE_SIZE_MB(and optionallyMAX_FILE_SIZE_BYTES) allows the UI to dynamically display the limit in error messages and documentation.Apply this diff:
-const MAX_FILE_SIZE_MB = 10; -const MAX_FILE_SIZE_BYTES = MAX_FILE_SIZE_MB * 1024 * 1024; +export const MAX_FILE_SIZE_MB = 10; +export const MAX_FILE_SIZE_BYTES = MAX_FILE_SIZE_MB * 1024 * 1024;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/layout/ImageUpload/imageUploadUtils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/layout/ImageUpload/imageUploadUtils.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Install
- GitHub Check: Analyze (javascript)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/language/texts/en.ts(1 hunks)src/language/texts/nb.ts(1 hunks)src/language/texts/nn.ts(1 hunks)src/layout/ImageUpload/ImageUploadSummary2.module.css(1 hunks)src/layout/ImageUpload/ImageUploadSummary2.tsx(1 hunks)src/layout/Option/OptionSummary.tsx(1 hunks)src/layout/Summary2/CommonSummaryComponents/SingleValueSummary.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/layout/Option/OptionSummary.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/layout/ImageUpload/ImageUploadSummary2.tsx
- src/language/texts/nn.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/layout/Summary2/CommonSummaryComponents/SingleValueSummary.tsxsrc/language/texts/nb.tssrc/language/texts/en.ts
**/*.module.css
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and follow existing patterns in
*.module.cssfiles
Files:
src/layout/ImageUpload/ImageUploadSummary2.module.css
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Install
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
🔇 Additional comments (4)
src/layout/Summary2/CommonSummaryComponents/SingleValueSummary.tsx (2)
15-15: LGTM! Type broadening enables richer summary content.The change from
stringtostring | React.ReactNodecorrectly supports displaying complex content (like images) in summaries, which aligns with the ImageUpload feature.
47-47: Verify falsy check behavior with ReactNode edge cases.With
displayDatanow acceptingReact.ReactNode, the falsy check!displayDataon lines 47 and 55 may produce unexpected results for edge cases:
null: valid ReactNode, currently treated as empty (likely desired)0: valid ReactNode that renders "0", currently treated as empty (may be unintended)false: valid ReactNode, currently treated as empty (likely desired)For typical use cases (strings, JSX elements), the logic works correctly. However, if numeric values like
0are ever displayed in summaries, they would incorrectly show the empty field text.Consider this more explicit check if numeric zero should be treated as valid content:
- [classes.emptyValue]: !displayData, + [classes.emptyValue]: displayData === undefined || displayData === null || displayData === '',- {!displayData && <Lang id={emptyFieldText ?? 'general.empty_summary'} />} + {(displayData === undefined || displayData === null || displayData === '') && <Lang id={emptyFieldText ?? 'general.empty_summary'} />}Also applies to: 55-56
src/language/texts/nb.ts (1)
212-217: LGTM!The Norwegian translations are clear, accurate, and properly aligned with the component's functionality (10MB limit, supported file formats).
Also applies to: 219-220
src/language/texts/en.ts (1)
211-220: LGTM!The English translations are clear, concise, and accurately reflect the component's constraints (10MB limit, supported file formats).
|
/publish |
PR release:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/layout/ImageUpload/imageUploadUtils.ts (4)
34-45: Use natural image dimensions; handle zero to avoid Infinity/NaN.This issue was flagged in a previous review:
HTMLImageElement.width/heightreturns CSS pixels, but pixel-accurate canvas calculations requirenaturalWidth/naturalHeight. Additionally, guard against zero dimensions to preventInfinity/NaNin clamp calculations.Refer to the previous review comment for the complete fix.
54-61: Use natural image dimensions; handle zero to avoid Infinity/NaN.This function has the same issue flagged in the previous review: use
naturalWidth/naturalHeightinstead ofwidth/heightfor accurate canvas calculations, and guard against zero dimensions.Refer to the previous review comment for the complete fix.
98-115: Guard zoom math against invalid ranges; clamp and avoid NaN/∞.This issue was flagged in a previous review:
Math.logwith non-positiveminZoom/maxZoom/valueyields-Infinity/NaN. The code needs input validation and output clamping to keep the slider domain stable.Refer to the previous review comment for the complete fix.
118-119: Use natural image dimensions; handle zero to avoid Infinity/NaN.This function has the same issue flagged in the previous review: use
naturalWidth/naturalHeightinstead ofwidth/height, and guard against zero dimensions to prevent returningInfinity.Refer to the previous review comment for the complete fix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/language/texts/en.ts(1 hunks)src/language/texts/nb.ts(2 hunks)src/language/texts/nn.ts(2 hunks)src/layout/ImageUpload/ImageControllers.tsx(1 hunks)src/layout/ImageUpload/ImageCropper.tsx(1 hunks)src/layout/ImageUpload/ImageDropzone.tsx(1 hunks)src/layout/ImageUpload/ImageUploadSummary2.test.tsx(1 hunks)src/layout/ImageUpload/imageUploadUtils.test.tsx(1 hunks)src/layout/ImageUpload/imageUploadUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/language/texts/nn.ts
- src/layout/ImageUpload/ImageUploadSummary2.test.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/layout/ImageUpload/ImageCropper.tsxsrc/layout/ImageUpload/ImageControllers.tsxsrc/layout/ImageUpload/ImageDropzone.tsxsrc/language/texts/nb.tssrc/layout/ImageUpload/imageUploadUtils.tssrc/layout/ImageUpload/imageUploadUtils.test.tsxsrc/language/texts/en.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProvidersfromsrc/test/renderWithProviders.tsxto supply required form layout context
Files:
src/layout/ImageUpload/imageUploadUtils.test.tsx
🧬 Code graph analysis (4)
src/layout/ImageUpload/ImageCropper.tsx (7)
src/layout/ImageUpload/imageUploadUtils.ts (8)
CropArea(8-8)Position(1-1)calculateMinZoom(118-119)constrainToArea(34-45)imagePlacement(54-61)validateFile(121-144)cropAreaPlacement(66-70)drawCropArea(79-87)src/layout/ImageUpload/useImageFile.tsx (1)
useImageFile(17-52)src/layout/ImageUpload/ImageDropzone.tsx (1)
ImageDropzone(18-58)src/app-components/Card/Card.tsx (1)
AppCard(20-54)src/layout/ImageUpload/ImageCanvas.tsx (1)
ImageCanvas(25-161)src/layout/ImageUpload/ImageControllers.tsx (1)
ImageControllers(25-155)src/features/language/Lang.tsx (1)
Lang(15-23)
src/layout/ImageUpload/ImageControllers.tsx (4)
src/features/attachments/index.ts (1)
UploadedAttachment(20-20)src/features/language/useLanguage.ts (1)
useLanguage(90-93)src/layout/ImageUpload/imageUploadUtils.ts (3)
normalToLogZoom(104-107)isAnimationFile(146-149)logToNormalZoom(109-115)src/features/language/Lang.tsx (1)
Lang(15-23)
src/layout/ImageUpload/ImageDropzone.tsx (4)
src/app-components/Dropzone/Dropzone.tsx (2)
IDropzoneProps(15-27)Dropzone(31-99)src/hooks/useDeviceWidths.ts (1)
useIsMobileOrTablet(33-35)src/components/label/Label.tsx (1)
getDescriptionId(135-141)src/features/language/Lang.tsx (1)
Lang(15-23)
src/layout/ImageUpload/imageUploadUtils.test.tsx (1)
src/layout/ImageUpload/imageUploadUtils.ts (10)
getCropArea(10-25)constrainToArea(34-45)imagePlacement(54-61)cropAreaPlacement(66-70)drawCropArea(79-87)normalToLogZoom(104-107)logToNormalZoom(109-115)calculateMinZoom(118-119)validateFile(121-144)isAnimationFile(146-149)
🔇 Additional comments (6)
src/layout/ImageUpload/imageUploadUtils.ts (6)
1-25: LGTM! Clean type definitions and getCropArea logic.The type definitions are precise, and the
getCropAreafunction correctly enforces the circle constraint (width = height) by using the minimum dimension when the type isCircle.
66-70: LGTM! Correct centering calculation.The function correctly centers the crop area within the canvas.
79-87: LGTM! Correct canvas drawing for both crop shapes.The function properly handles both circle (using
arc) and square (usingrect) crop areas.
121-144: LGTM! File validation logic is correct.The validation correctly checks:
- File presence
- Size limit (10 MB)
- Image type verification
The constants
MAX_FILE_SIZE_MBandMAX_FILE_SIZE_BYTESare currently scoped inside the function. If these need to be referenced externally (as suggested by the AI summary), consider exporting them at the module level.
146-149: LGTM! Animation detection logic is correct.The function properly detects animation mime types with case-insensitive matching.
1-1: Note: Past review comment aboutgetAcceptedFilesdoesn't apply to this file.One of the past review comments mentions a
getAcceptedFilesfunction with accept string validation issues. This function is not present inimageUploadUtils.ts. The comment may refer to a different file or the function may have been moved elsewhere.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/layout/ImageUpload/ImageCropper.tsx (2)
99-114: Clean up pending Image/FileReader to avoid leaks and stale callbacks
When replacing files or unmounting mid‑load, the previous Image can continue loading. Track and cancel previous loaders; guardonloadagainst stale updates.+ const loadingImageRef = useRef<HTMLImageElement | null>(null); + const fileReaderRef = useRef<FileReader | null>(null); const handleFileUpload = (file: File) => { const validationErrors = validateFile(file); setValidationErrors(validationErrors); if (validationErrors.length > 0) { return; } - - const reader = new FileReader(); + // Cleanup any in-flight loaders + if (loadingImageRef.current) { + loadingImageRef.current.onload = null; + loadingImageRef.current.src = ''; + loadingImageRef.current = null; + } + if (fileReaderRef.current && 'readyState' in fileReaderRef.current) { + try { fileReaderRef.current.abort?.(); } catch {} + } + const reader = new FileReader(); + fileReaderRef.current = reader; reader.onload = (event) => { const result = event.target?.result; if (typeof result === 'string') { const img = new Image(); + loadingImageRef.current = img; img.id = file.name; imageTypeRef.current = file.type; img.onload = () => { - updateImageState({ minZoom: calculateMinZoom({ img, cropArea }), img }); + if (loadingImageRef.current === img) { + updateImageState({ minZoom: calculateMinZoom({ img, cropArea }), img }); + loadingImageRef.current = null; + } }; img.src = result; } }; reader.readAsDataURL(file); }; + React.useEffect(() => () => { + if (loadingImageRef.current) { + loadingImageRef.current.onload = null; + loadingImageRef.current.src = ''; + } + try { fileReaderRef.current?.abort?.(); } catch {} + }, []);
136-144: Handle canvas.toBlob failure and report to userIf
toBlobreturns null, provide feedback; also wrap File creation to catch exceptions.- cropCanvas.toBlob((blob) => { - if (!blob) { - return; - } - const fileName = img.id; - const imageFile = new File([blob], fileName, { type: 'image/png' }); - saveImage(imageFile); - setValidationErrors(null); - }, 'image/png'); + cropCanvas.toBlob((blob) => { + if (!blob) { + console.error('Failed to create image blob'); + setValidationErrors(['image_upload_component.error_generate_image_failed']); + return; + } + try { + const fileName = img.id; + const imageFile = new File([blob], fileName, { type: 'image/png' }); + saveImage(imageFile); + setValidationErrors(null); + } catch (e) { + console.error('Failed to save image', e); + setValidationErrors(['image_upload_component.error_generate_image_failed']); + } + }, 'image/png');src/layout/ImageUpload/ImageControllers.tsx (2)
45-53: Fix zoom slider normalization (broken zoom and thumb position)Utils expect 0–1, slider is 0–100. Divide input by 100; multiply value by 100.
const handleSliderZoom = (e: React.ChangeEvent<HTMLInputElement>) => { const logarithmicZoomValue = normalToLogZoom({ - value: Number.parseFloat(e.target.value), + value: Number.parseFloat(e.target.value) / 100, minZoom, maxZoom, }); @@ <input id={zoomId} type='range' min='0' max='100' step='0.5' - value={logToNormalZoom({ value: zoom, minZoom, maxZoom })} + value={logToNormalZoom({ value: zoom, minZoom, maxZoom }) * 100} onChange={handleSliderZoom} className={classes.zoomSlider} />Also applies to: 89-96
99-109: Respect readOnly and add accessible name for Reset; simplify “Change” to a real button
- Disable Save/Reset/Change/Cancel when readOnly.
- Provide aria-label for the icon-only Reset button.
- Avoid
asChildwith a Label; use a real Button that clicks the input ref so disabled works.
[accessibility]- <Button - onClick={onReset} - variant='tertiary' - icon={true} - > - <ArrowUndoIcon - title={langAsString('image_upload_component.reset')} - className={classes.resetButton} - /> - </Button> + <Button + onClick={onReset} + variant='tertiary' + icon + aria-label={langAsString('image_upload_component.button_reset')} + disabled={readOnly} + > + <ArrowUndoIcon className={classes.resetButton} /> + </Button> @@ <Button onClick={onSave} data-size='sm' data-color='accent' + disabled={readOnly} > <Lang id='image_upload_component.button_save' /> </Button> @@ - <Button - asChild - data-size='sm' - variant='secondary' - data-color='accent' - tabIndex={0} - onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault(); - fileInputRef?.current?.click(); - } - }} - > - <Label htmlFor={inputId}> - <UploadIcon /> - <Lang id='image_upload_component.button_change' /> - </Label> - </Button> + <Button + data-size='sm' + variant='secondary' + data-color='accent' + onClick={() => fileInputRef.current?.click()} + disabled={readOnly} + > + <UploadIcon /> + <Lang id='image_upload_component.button_change' /> + </Button> <Button data-size='sm' variant='tertiary' onClick={onCancel} data-color='accent' + disabled={readOnly} > <Lang id='general.cancel' /> </Button>Also applies to: 112-118, 128-145, 146-153
🧹 Nitpick comments (3)
src/layout/ImageUpload/ImageUploadComponent.tsx (2)
33-36: Remove unnecessary type cast on getCropArea inputThe object passed to getCropArea matches its expected params; the
as CropAreacast is unnecessary and misleads types. Drop the cast.
[typescript] [As per coding guidelines]- cropArea={getCropArea({ width: cropWidth, height: cropHeight, type: cropShape } as CropArea)} + cropArea={getCropArea({ width: cropWidth, height: cropHeight, type: cropShape })}
21-36: Verify that Label htmlFor matches the interactive control idLabel uses
htmlFor={id}, while ImageDropzone setsid={baseComponentId}. In repeating groups, these can differ (indexed id). Ensure the same id is used for both to keep label association and a11y intact (pass the actual control id down to ImageCropper/ImageDropzone if needed).If misaligned, prefer passing the indexed id from useLabel/useIndexedId to Dropzone.
src/layout/ImageUpload/ImageControllers.tsx (1)
123-125: Align accepted file types with policy (extensions/types)Restrict accept to the allowed types from the PR: png, jpg/jpeg, heic, webp.
- accept='image/*' + accept='.png,.jpg,.jpeg,.heic,.webp,image/png,image/jpeg,image/heic,image/webp'Please confirm HEIC support in your target browsers/environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/layout/ImageUpload/ImageControllers.tsx(1 hunks)src/layout/ImageUpload/ImageCropper.tsx(1 hunks)src/layout/ImageUpload/ImageUploadComponent.test.tsx(1 hunks)src/layout/ImageUpload/ImageUploadComponent.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/layout/ImageUpload/ImageUploadComponent.test.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/layout/ImageUpload/ImageCropper.tsxsrc/layout/ImageUpload/ImageControllers.tsxsrc/layout/ImageUpload/ImageUploadComponent.tsx
🧬 Code graph analysis (3)
src/layout/ImageUpload/ImageCropper.tsx (7)
src/layout/ImageUpload/imageUploadUtils.ts (8)
CropArea(8-8)Position(1-1)calculateMinZoom(118-119)constrainToArea(34-45)imagePlacement(54-61)validateFile(121-144)cropAreaPlacement(66-70)drawCropArea(79-87)src/layout/ImageUpload/useImageFile.tsx (1)
useImageFile(17-52)src/layout/ImageUpload/ImageDropzone.tsx (1)
ImageDropzone(18-58)src/app-components/Card/Card.tsx (1)
AppCard(20-54)src/layout/ImageUpload/ImageCanvas.tsx (1)
ImageCanvas(25-161)src/layout/ImageUpload/ImageControllers.tsx (1)
ImageControllers(26-157)src/features/language/Lang.tsx (1)
Lang(15-23)
src/layout/ImageUpload/ImageControllers.tsx (4)
src/features/attachments/index.ts (1)
UploadedAttachment(20-20)src/features/language/useLanguage.ts (1)
useLanguage(90-93)src/layout/ImageUpload/imageUploadUtils.ts (3)
normalToLogZoom(104-107)isAnimationFile(146-149)logToNormalZoom(109-115)src/features/language/Lang.tsx (1)
Lang(15-23)
src/layout/ImageUpload/ImageUploadComponent.tsx (6)
src/layout/index.ts (1)
PropsFromGenericComponent(29-33)src/utils/layout/useNodeItem.ts (1)
useItemWhenType(15-33)src/utils/layout/useLabel.tsx (1)
useLabel(13-72)src/layout/ComponentStructureWrapper.tsx (1)
ComponentStructureWrapper(20-48)src/layout/ImageUpload/ImageCropper.tsx (1)
ImageCropper(29-213)src/layout/ImageUpload/imageUploadUtils.ts (2)
getCropArea(10-25)CropArea(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Install
- GitHub Check: Analyze (javascript)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/layout/ImageUpload/ImageUploadComponent.tsx (1)
35-35: Consider simplifying boolean coercion.The
!!readOnlydouble negation is defensive but unnecessary ifreadOnlyis already typed asboolean | undefinedandImageCropperacceptsreadOnly?: boolean. Simply passingreadOnlywould be cleaner.Apply this diff if the types align:
- readOnly={!!readOnly} + readOnly={readOnly}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/layout/FileUpload/FileUploadComponent.test.tsx(1 hunks)src/layout/ImageUpload/ImageUploadComponent.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/layout/FileUpload/FileUploadComponent.test.tsxsrc/layout/ImageUpload/ImageUploadComponent.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProvidersfromsrc/test/renderWithProviders.tsxto supply required form layout context
Files:
src/layout/FileUpload/FileUploadComponent.test.tsx
🧬 Code graph analysis (1)
src/layout/ImageUpload/ImageUploadComponent.tsx (6)
src/layout/index.ts (1)
PropsFromGenericComponent(29-33)src/utils/layout/useNodeItem.ts (1)
useItemWhenType(15-33)src/utils/layout/useLabel.tsx (1)
useLabel(13-72)src/layout/ComponentStructureWrapper.tsx (1)
ComponentStructureWrapper(20-48)src/layout/ImageUpload/ImageCropper.tsx (1)
ImageCropper(29-213)src/layout/ImageUpload/imageUploadUtils.ts (1)
getCropArea(10-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Install
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
🔇 Additional comments (2)
src/layout/FileUpload/FileUploadComponent.test.tsx (1)
476-476: LGTM! Translation refinement.The test expectation has been correctly updated to reflect the refined Norwegian text from "let etter fil" to "finn fil". This aligns with the updated UI text in the Dropzone component.
src/layout/ImageUpload/ImageUploadComponent.tsx (1)
12-16: LGTM! readOnly now propagated correctly.The component now correctly destructures
readOnlyfromuseItemWhenTypeand passes it through toImageCropper(line 35), addressing the previous review concern.
|
| 'image_upload_component.summary_empty': "You haven't uploaded an image", | ||
| 'image_upload_component.reset': 'Reset position and zoom', | ||
| 'image_upload_component.error_invalid_file_type': 'Invalid file format. Please upload an image file.', | ||
| 'image_upload_component.error_file_size_exceeded': 'File size exceeds 10MB limit.', |
There was a problem hiding this comment.
I checked, and I have some JPEGs straight out of my camera that are larger than this. Is there any reason for this limit? Won't this image become way smaller after cropping and resolution-limiting anyway?
There was a problem hiding this comment.
You are correct, the cropped image that is saved becomes way smaller. But it seems like the browser doesn't handle it well when reading larger files, such as 50mbs. This happens when javascript tries to decode the full image into memory and allocate a texture in gpu memory when drawing the image in canvas that is used to display the cropping area. And during this both versions is stored temporarily.
const handleFileUpload = (file: File) => {
const validationErrors = validateFile(file);
setValidationErrors(validationErrors);
if (validationErrors.length > 0) {
return;
}
const reader = new FileReader();
reader.onload = (event) => {
const result = event.target?.result;
if (typeof result === 'string') {
const img = new Image();
img.id = file.name;
imageTypeRef.current = file.type;
img.onload = () => {
updateImageState({ minZoom: calculateMinZoom({ img, cropArea }), img });
};
img.src = result;
}
};
reader.readAsDataURL(file);
};
After the load. the browser keeps struggling. So we ended up setting a limit on 10mb to ensure a good user experience on different browsers. With this context in mind, profile images, we thought that most images would probably would be below 10mb. But I tested a bit more, and it seem it maybe can handle up to 20mbs as well with a bit delay at the beginning.
What do you think?
There was a problem hiding this comment.
Ah, I see! I thought it might just be an arbitrary limit. It really comes down to your device then. Will it destroy performance even after cropping and uploading (or clearing the image), or is it permanent? If it's slow forever after I'm in favor of a limit, but otherwise maybe an alert is fine (something like the alert for animations)?
There was a problem hiding this comment.
It seems like it works fine after I manage to save it, but before that everything lags enormously 😄 Panning the cropping area or using zoom can be extremely slow, and one time I got a browser error. I tested with 50mb from https://examplefile.com/image/jpg.
I also tried to upload this kind of image on linkedin, which uses canvas too, and I experienced the same problem there. On linkedin the image hasn't been displayed yet after more than 5 minutes. So our solution handles it better 🥳
As you mention, it could be an good option to display an alert message similar to animations if it exceeds 10mbs (something like: "Bildet er større enn 10 MB. Du kan oppleve at redigering/nettleseren går saktere enn vanlig.").
I’m still a bit in favor of limiting the size, since the UX overall is pretty bad until the user manages to save, but I can sleep on it.
| .addProperty( | ||
| new CG.prop( | ||
| 'cropShape', | ||
| new CG.enum('square', 'circle') |
There was a problem hiding this comment.
The 'square' setting can also be a rectangle, so that might be a better naming. It's only a square if you set cropWidth and cropHeight to the same values, but if you specify different values you'll make a rectangle, not a square.
There was a problem hiding this comment.
Agree, will change it.
| .addProperty( | ||
| new CG.prop( | ||
| 'cropWidth', | ||
| new CG.num().optional({ default: 250 }).setTitle('Width').setDescription('Optional width of the cropping area'), | ||
| ), | ||
| ) | ||
| .addProperty( | ||
| new CG.prop( | ||
| 'cropHeight', | ||
| new CG.num().optional({ default: 250 }).setTitle('Height').setDescription('Optional height of the cropping area'), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
You might want to add the detail that for 'circle' only the smallest of these two actually have an effect. Thus, maybe 'circle' is not as great as a default value - since these two options have weird effects? You could potentially also add a union type that removes the gotchas in this config, such as only allowing objects:
"crop": {
"shape": "circle",
"diameter": 250
}"crop": {
"shape": "rectangle",
"width": 250,
"height": 250
}Example of how to do this in config.ts:
Config.addProperty(
new CG.prop(
'crop',
new CG.union(
new CG.obj(
new CG.prop('shape', new CG.const('circle').setTitle('Shape').setDescription('Circular cropping area')),
new CG.prop(
'diameter',
new CG.num().optional({ default: 250 }).setTitle('Diameter').setDescription('Diameter of the circle'),
),
).exportAs('CropConfigCircle'),
new CG.obj(
new CG.prop('shape', new CG.const('rectangle').setTitle('Shape').setDescription('Rectangular cropping area')),
new CG.prop(
'width',
new CG.num().optional({ default: 250 }).setTitle('Width').setDescription('Width of the rectangle'),
),
new CG.prop(
'height',
new CG.num().optional({ default: 250 }).setTitle('Height').setDescription('Height of the rectangle'),
),
).exportAs('CropConfigRect'),
)
.setUnionType('discriminated')
.optional({ default: { shape: 'circle', diameter: 250 } })
.exportAs('CropConfig'),
),
)There was a problem hiding this comment.
Nice suggestion! 👍 I will take a look at it, and see how we can display this in studio designer as well. For now we have a selector for shape and 2 input fields for width and height.
| const handleWheel = useCallback( | ||
| (e: WheelEvent) => { | ||
| e.preventDefault(); | ||
| onZoomChange(zoom - e.deltaY * 0.001); |
There was a problem hiding this comment.
Hmm, I found this to be less useful than I hoped. The first scroll 'click' zooms in waay too much, but later clicks zoom in less and less. I didn't need all that finesse when looking at huge pixels, but I wanted more finesse at first.. 😅
There was a problem hiding this comment.
Ah, you mean it should be less zoom sensitive in the first part of the zoom scale when working with huge pixels?
There was a problem hiding this comment.
haha, I see what you mean 😆 Here I scroll:
zoom.mp4
| const imageFile = new File([blob], fileName, { type: 'image/png' }); | ||
| saveImage(imageFile); | ||
| setValidationErrors(null); | ||
| }, 'image/png'); |
There was a problem hiding this comment.
Oh, nice! Looks very easy to add an option to upload as jpeg as well (seems supported in all major browsers). I see lots of apps have added limitations in the allowedContentTypes property for the data element type in applicationmetadata, so maybe we should check that and upload as image/jpeg if image/png is not allowed by the app. Aaaand throw an error early if none of the supported image types are allowed, I guess.
There was a problem hiding this comment.
Good point, will look into that 🙂
| background-image: | ||
| linear-gradient( | ||
| 45deg, | ||
| rgba(204, 204, 204, 0.5) 25%, | ||
| transparent 25%, | ||
| transparent 75%, | ||
| rgba(204, 204, 204, 0.5) 75%, | ||
| rgba(204, 204, 204, 0.5) 100% | ||
| ), | ||
| linear-gradient( | ||
| 45deg, | ||
| rgba(204, 204, 204, 0.5) 25%, | ||
| rgba(255, 255, 255, 0.5) 25%, | ||
| rgba(255, 255, 255, 0.5) 75%, | ||
| rgba(204, 204, 204, 0.5) 75%, | ||
| rgba(204, 204, 204, 0.5) 100% | ||
| ); |
There was a problem hiding this comment.
hehe, i know. I'm not sure how to make that simpler 😅
There was a problem hiding this comment.
No need! It was pretty cool! 🥇
| } | ||
|
|
||
| useDataModelBindingValidation(baseComponentId: string, bindings: IDataModelBindings<'ImageUpload'>): string[] { | ||
| return useFileUploaderDataBindingsValidation(baseComponentId, bindings); |
There was a problem hiding this comment.
That hook should probably also be updated to allow ImageUpload bindings as an input
| } | ||
|
|
||
| renderSummary(_props: SummaryRendererProps): JSX.Element | null { | ||
| throw new Error('Method not implemented.'); // Use renderSummary2, will probably not be supported |
There was a problem hiding this comment.
Best to write that comment in the error thrown as well - the comment won't be displayed to app developers, but the exception text will
| } | ||
|
|
||
| .previewBackground { | ||
| background-color: #f4f5f6; /* Following does not exist in v1: var(--ds-color-neutral-background-subtle); */ |
There was a problem hiding this comment.
Should we use a different color here? If this color already exists elsewhere, we should align with that and reference a shared CSS variable/design token. If it’s specific to this component only, consider introducing a v1 component-scoped token instead of hard-coding the value.
| if (storedImage) { | ||
| return ( | ||
| <div className={classes.previewBackground}> | ||
| {storedImage.uploaded ? ( | ||
| <img | ||
| src={imageUrl} | ||
| alt={storedImage.data?.filename} | ||
| /> | ||
| ) : ( | ||
| <Spinner | ||
| aria-hidden='true' | ||
| data-size='lg' | ||
| aria-label={langAsString('general.loading')} | ||
| /> | ||
| )} | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Should we extract the JSX inside this if into a separate component, e.g. UploadedImagePreview?
| if (storedImage) { | |
| return ( | |
| <div className={classes.previewBackground}> | |
| {storedImage.uploaded ? ( | |
| <img | |
| src={imageUrl} | |
| alt={storedImage.data?.filename} | |
| /> | |
| ) : ( | |
| <Spinner | |
| aria-hidden='true' | |
| data-size='lg' | |
| aria-label={langAsString('general.loading')} | |
| /> | |
| )} | |
| </div> | |
| ); | |
| } | |
| if (storedImage) { | |
| return <UploadedImagePreview | |
| storedImage={storedImage} | |
| imageUrl={imageUrl} | |
| />; | |
| } |
| height={CANVAS_HEIGHT} | ||
| width={CANVAS_WIDTH} | ||
| className={classes.canvas} | ||
| aria-label='Image cropping area' |
There was a problem hiding this comment.
I think we should support multiple languages for this aria-label? 🤔
| @@ -0,0 +1,161 @@ | |||
| import React, { useCallback, useEffect } from 'react'; | |||
There was a problem hiding this comment.
Great work on this component! This isn’t a blocker, but it currently mixes concerns, Single Responsibility Principle. It handles rendering, event handling, drag-and-drop, zoom, keyboard navigation, and canvas drawing in one place. Consider splitting these out to improve readability and testability.
An alternative is to split responsibilities into separate modules. I’m not suggesting we scatter logic—maintaining cohesion matters—but we can keep everything under the component’s folder and factor it into focused files.
ImageCanvas.tsx
├── hooks/
│ ├──hooks.something.ts
├── utils/
│ ├── SomeUtiLS
The ImageCanvas could look like the example below. We’re not writing unit tests with Vitest or Jest right now, so most coverage sits in Cypress so its easy to test as is.
export function ImageCanvas({
imageRef,
zoom,
position,
cropArea,
baseComponentId,
onPositionChange,
onZoomChange,
canvasRef,
}: ImageCanvasProps) {
const { storedImage, imageUrl } = useImageFile(baseComponentId);
// extract canvas and zoom into custom hooks.
useCanvasDraw({ canvasRef, imageRef, zoom, position, cropArea });
useZoomInteraction(canvasRef, zoom, onZoomChange);
const { handlePointerDown } = useDragInteraction(canvasRef, position, onPositionChange);
const { handleKeyDown } = useKeyboardNavigation(position, onPositionChange);
// Ref. previous comment, the code below seperated the preview to explicit component.
if (storedImage) {
return <UploadedImagePreview storedImage={storedImage} imageUrl={imageUrl} />;
}
return (
<canvas
onPointerDown={handlePointerDown}
onKeyDown={handleKeyDown}
tabIndex={0}
ref={canvasRef}
height={CANVAS_HEIGHT}
width={CANVAS_WIDTH}
className={classes.canvas}
aria-label='Image cropping area' // <-- Ref. previous comment, this should support multiple langues.
/>
);
}| /* ✅ Chrome / Safari / Edge */ | ||
| .zoomSlider::-webkit-slider-thumb { | ||
| -webkit-appearance: none; | ||
| appearance: none; | ||
| width: var(--ds-size-8); | ||
| height: var(--ds-size-8); | ||
| border-radius: var(--ds-border-radius-full); | ||
| border: 1px solid var(--ds-color-border-strong); | ||
| background: var(--ds-color-base-contrast-default); | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| /* ✅ Firefox */ | ||
| .zoomSlider::-moz-range-thumb { | ||
| width: var(--ds-size-8); | ||
| height: var(--ds-size-8); | ||
| border-radius: var(--ds-border-radius-full); | ||
| border: 1px solid var(--ds-color-border-strong); | ||
| background: var(--ds-color-base-contrast-default); | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| /* ✅ Firefox track (to override default blue bar) */ |
There was a problem hiding this comment.
Do we need to have emojis in our comments here? 😊
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault(); | ||
| fileInputRef?.current?.click(); | ||
| } |
There was a problem hiding this comment.
nit: should this be extracted to a function instead of inline? 😊
| box-sizing: border-box; | ||
| } | ||
|
|
||
| .blueUnderLine { |
There was a problem hiding this comment.
nit: "blue" is more implementation detail, not that important for a classname?
| .blueUnderLine { | |
| .underLine { |
| export const imagePlacement = ({ canvas, img, zoom, position }: ImagePlacementParams) => { | ||
| const scaledWidth = img.width * zoom; | ||
| const scaledHeight = img.height * zoom; | ||
| const imgX = (canvas.width - scaledWidth) / 2 + position.x; | ||
| const imgY = (canvas.height - scaledHeight) / 2 + position.y; | ||
|
|
||
| return { imgX, imgY, scaledWidth, scaledHeight }; | ||
| }; | ||
|
|
||
| type CropAreaPlacementParams = { canvas: HTMLCanvasElement; cropArea: CropArea }; | ||
| type CropAreaPlacement = { cropAreaX: number; cropAreaY: number }; | ||
|
|
||
| export const cropAreaPlacement = ({ canvas, cropArea }: CropAreaPlacementParams): CropAreaPlacement => { | ||
| const cropAreaX = (canvas.width - cropArea.width) / 2; | ||
| const cropAreaY = (canvas.height - cropArea.height) / 2; | ||
| return { cropAreaX, cropAreaY }; | ||
| }; | ||
|
|
||
| interface DrawCropAreaParams { | ||
| ctx: CanvasRenderingContext2D; | ||
| cropArea: CropArea; | ||
| x?: number; | ||
| y?: number; | ||
| } | ||
|
|
||
| export function drawCropArea({ ctx, x = 0, y = 0, cropArea }: DrawCropAreaParams) { | ||
| const { width, height, type } = cropArea; | ||
| ctx.beginPath(); | ||
| if (type === CropForm.Circle) { | ||
| ctx.arc(x + width / 2, y + height / 2, width / 2, 0, Math.PI * 2); | ||
| } else { | ||
| ctx.rect(x, y, width, height); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we extract the magic numbers into named constants? I notice the value 2 appears multiple times—do these instances represent the same domain rule, or are they unrelated?
Description
New component ImageUpload. The main functionality of this component is to let the user upload an image and crop it before saving it to the instance. When it is saved, user will be displayed the result.
Validations are added: the file must be of type image and the size can't exceed 10MB. If user adds an image type that can contain animation such as
gif, apng or webp, we inform user that only the first frame is displayed.The logic behind cropping the image, which is also used in other third-party libraries, uses the web API Canvas. It can draw the uploaded image according to our specifications, such as which part of the image should be drawn and kept inside the crop area.
The content of the crop area is determined by various utilities that calculate the position of the image (x, y) relative to the canvas, the size of the image relative to the canvas, the center of the crop area, clamped positions (restricting the user from dragging the crop area outside the image), etc. Most of these calculation functions can be found in the utility file.
This new component has some specific configuration options:
cropShape: enum["square", "circle"], where "circle" is the default.cropWidth: the width of the crop area in pixels. Default is 250px.cropHeight: the height of the crop area in pixels. Default is also 250px.These are optional props. If the user chooses not to set them, they will default to 250px and "circle".
Studio also supports this new component and is currently behind the feature flag:
imageUpload.This component also supports keyboard users.
Altinn docs is updated.
Here is how the component will behave for the user:
imageupload.mp4
Related Issue(s)
Verification/QA
docs for new component - image upload altinn-studio-docs#2388
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
New Features
Behavior
Localization
Style
Tests