[WC-3337]: Pluggable Image Crop Widget#2235
Conversation
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds getPreview structure tile (icon + caption with shape/aspect/output), fit-and-scale canvas sizing in CropArea (no more gaps after horizontal crop), DPR-aware PreviewPane buffer, exhaustiveness guard in aspectRatio, and required="true" on three XML booleans. SVG asset replaces inline data URL. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rahmanunver
changed the title
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds getPreview structure tile (icon + caption with shape/aspect/output), fit-and-scale canvas sizing in CropArea (no more gaps after horizontal crop), DPR-aware PreviewPane buffer, exhaustiveness guard in aspectRatio, and required="true" on three XML booleans. SVG asset replaces inline data URL. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rahmanunver
force-pushed
the
feat/image-crop-web
branch
from
bcd4ea9 to
b19bb26
Compare
This comment was marked as outdated.
This comment was marked as outdated.
AI Code Review
What was reviewed
Skipped (out of scope): Findings🔶 Medium —
|
| <enumerationValue key="onWithCtrl">On (hold Ctrl)</enumerationValue> | ||
| </enumerationValues> | ||
| </property> | ||
| <property key="minZoom" type="decimal" defaultValue="1" required="true"> |
There was a problem hiding this comment.
Decimal is intentional here. Zoom is a multiplier (1× = image fits the canvas, 2× = double, 0.5× = half-size). Real-world configurations need fractional values:
- minZoom: 0.5 — lets users zoom out further than fit, useful when the source image is larger than the canvas and the user wants to see the whole thing before cropping.
- maxZoom: 2.5 — caps zoom mid-step so the slider doesn't overshoot detail.
Forcing integers would round these to the nearest whole multiple, breaking the slider's natural feel and the wheel-zoom step.
| <caption>Output size</caption> | ||
| <description>Resolution of the saved crop. Original is sharpest; Viewport matches the on-screen canvas size.</description> | ||
| <enumerationValues> | ||
| <enumerationValue key="viewport">Viewport (canvas dims)</enumerationValue> |
There was a problem hiding this comment.
Will rename to dimensions
| <caption>Crop button caption</caption> | ||
| <description>Label on the Crop button.</description> | ||
| <translations> | ||
| <translation lang="en_US">Crop</translation> |
| const _exhaustive: never = aspect; | ||
| return _exhaustive; |
There was a problem hiding this comment.
TypeScript exhaustiveness guard;
Imagine adding a new case, a new aspectRatio in the .xml, but don't add the new case in this switch;
After all existing cases are ruled out, since no case for the new aspect ratio exists, it will return undefined, causing the widget to act as if the setting was set to "free", and have no warnings.
A new "landscape21x9" is added as an option in the xml.
Adding a default case and setting the aspect to a value with type never:
error TS2322: Type '"landscape21x9"' is not assignable to type 'never'.
src/utils/aspectRatio.ts:25 const _exhaustive: never = aspect;
The build fails with the error pointing at exactly this line, and the developer is forced to add the missing case "landscape21x9": return 21 / 9; before they can ship.
| <div className="widget-image-crop__error"> | ||
| Image source does not allow cropping. Upload locally or configure CORS. | ||
| </div> |
There was a problem hiding this comment.
Not sure if this error message makes any sense.
| const setImageRef = useCallback( | ||
| (img: HTMLImageElement | null) => { | ||
| imageRef.current = img; | ||
| }, | ||
| [imageRef] | ||
| ); |
There was a problem hiding this comment.
Is it needed? Maybe we can pass imageRef as ref directly.
There was a problem hiding this comment.
Updating it, stale change.
| <div | ||
| ref={containerRef} | ||
| className={classNames("widget-image-crop__canvas", { | ||
| "widget-image-crop__canvas--circle": props.circular | ||
| })} | ||
| style={{ maxWidth: props.boundaryWidth, maxHeight: props.boundaryHeight }} | ||
| > |
There was a problem hiding this comment.
We can decouple container wrapper with zoom logic to a separate component.
There was a problem hiding this comment.
refactoring into a ZoomContainer
| disabled={!props.resizable} | ||
| locked={!props.resizable} |
There was a problem hiding this comment.
Should it really be locked if not resizable?
There was a problem hiding this comment.
Will drop, keep only disabled
| transformOrigin: "center" | ||
| }} | ||
| onLoad={handleImageLoad} | ||
| onError={(_e: SyntheticEvent<HTMLImageElement>) => setLoadError(true)} |
There was a problem hiding this comment.
You likely can use empty signature () =>
| const [crop, setCrop] = useState<Crop | undefined>(undefined); | ||
| const [completedCrop, setCompletedCrop] = useState<PixelCrop | undefined>(undefined); | ||
| const [zoom, setZoom] = useState<number>(initialZoom); | ||
| const imageRef = useRef<HTMLImageElement | null>(null); |
There was a problem hiding this comment.
useRef adds | null by default in this case. Check other places as well.
| crop: Crop | undefined; | ||
| setCrop: Dispatch<SetStateAction<Crop | undefined>>; | ||
| completedCrop: PixelCrop | undefined; | ||
| setCompletedCrop: Dispatch<SetStateAction<PixelCrop | undefined>>; |
There was a problem hiding this comment.
What is the difference between those, can we use only one instead of two?
There was a problem hiding this comment.
They fire on different events (onChange per pointer move vs onComplete on release), so collapsing them either makes the selection rectangle freeze during drag or makes the preview canvas re-render dozens of times per second.
| maxWidth: displaySize ? undefined : props.boundaryWidth, | ||
| maxHeight: displaySize ? undefined : props.boundaryHeight, | ||
| transform: `scale(${props.zoom})`, | ||
| transformOrigin: "center" |
There was a problem hiding this comment.
Do I understand it correctly that it only zooms into the center, like there is no way to zoom in and crop a corner of and image?
There was a problem hiding this comment.
That's for the zoom in/out either with gesture or slider, we have drag-to-crop for corner edge cropping (ss is for circle crop).
AI Code Review
What was reviewed
Skipped (out of scope): Findings🔶 Medium — Loading and empty states omit
|
rahmanunver
force-pushed
the
feat/image-crop-web
branch
from
b824245 to
f376009
Compare
AI Code Review
What was reviewed
Skipped (out of scope): Findings🔶 Medium —
|
2 participants
Pull request type
New feature (non-breaking change which adds functionality)
Description
Adds Image Crop, a new pluggable widget that lets end-users crop an image directly inside a Mendix page and saves the result back to an Image attribute.