Repository navigation
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Walkthrough
ChangesImageCrop sizing
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (2 skipped: 2… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check The active linked issues have no conflicting coding acceptance criteria. For #12, the PR removes the obsoletesetImageFullHeight(Boolean)API and documentssetMaxHeight(...)as the replacement. Fo…Out of Scope Changes check The reviewed changes stay within #8and#12. The Java API change, CSS rules, README example, and upload-demo change all support image sizing or the removal and replacement ofsetImageFullHeight. No …Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly summarizes the main change: constraining the image to the component's maximum height. The feat!marker also signals the breaking change caused by removingsetImageFullHeight.Full details: Linked Issues check
-
Explanation
The active linked issues have no conflicting coding acceptance criteria. For
#12, the PR removes the obsoletesetImageFullHeight(Boolean)API and documentssetMaxHeight(...)as the replacement. For#8,ImageCropnow implementsHasSize, the component hasmax-width: 100%, and the CSS propagatesmax-heightthrough.ReactCropto the image. The class documentation states that large images scale down with aspect ratio and small images are not upscaled. The README and upload demo document and usesetMaxHeight("70vh"). Neither issue specifies an automated-test requirement.
Full details: Out of Scope Changes check
-
Explanation
The reviewed changes stay within
#8and#12. The Java API change, CSS rules, README example, and upload-demo change all support image sizing or the removal and replacement ofsetImageFullHeight. No unrelated behavior or files are identified in the supplied PR evidence.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
javier-godoy
left a comment
javier-godoy
left a comment
There was a problem hiding this comment.
Implementing HasSize is an unrelated change
ea84365 to
ef5be8b
Compare
|
@javier-godoy Removed the |
| /* Propagate the component's max-height to the image (react-image-crop | ||
| applies max-height: inherit to its wrapper and to the image). */ | ||
| image-crop .ReactCrop { | ||
| max-height: inherit; |
There was a problem hiding this comment.
inherit copies the max-height value as written, not as computed, so a percentage doesn't work. With max-height: 100% on an image-crop inside a 300px-tall container, the host resolves it to 300px. .ReactCrop also gets 100%, but resolves it against the host, whose height is auto, so it acts as none. The wrapper and the img inherit that, and a 4000×3000 image renders at 1280×960, overflowing the host. The same layout with max-height: 300px works (the image renders at 400×300). The same happens with calc() values that contain a percentage, and with a Dialog sized setMaxHeight("100%") like the one in the upload demo.
If only absolute and viewport lengths are meant to be supported, the Javadoc and README should say so; right now they promise the image fits "the component's max-height".
There was a problem hiding this comment.
Fixed: the image is now fitted in the .tsx to the component's measured height, so percentages, calc(), fixed heights and layout sizes all work. Javadoc and README updated accordingly.
There was a problem hiding this comment.
Confirmed: in a 300px container, max-height: 100% and calc(100% - 20px) now render a 4000×3000 image at 400×300 and 373×280, and the image also fits in a Dialog with setMaxHeight("100%"). The docs now claim a bit more than the code does, though; see my comment on the README.
7e52563 to
8895cf0
Compare
|
| const available = this.clientHeight - parseFloat(style.paddingTop) - parseFloat(style.paddingBottom); | ||
| if (img.getBoundingClientRect().height > available + 0.5) { |
There was a problem hiding this comment.
getBoundingClientRect() includes CSS transforms, but clientHeight doesn't, and a transform change doesn't trigger the ResizeObserver. In a Dialog, the image is measured during the opening animation, while the overlay is scaled to 0.95. With max-height: 70vh at a 905px viewport (633.5px) and a 1000×650 image, the image measures 618px, so it isn't clamped. When the animation ends, it renders at 650px in a 634px component, overflowing by 17px, and nothing refits it. The same happens inside any container with a scale below 1. Comparing img.offsetHeight, which isn't affected by transforms, avoids this.
| cropElement.style.maxHeight = ""; | ||
| const style = getComputedStyle(this); | ||
| const available = this.clientHeight - parseFloat(style.paddingTop) - parseFloat(style.paddingBottom); | ||
| if (img.getBoundingClientRect().height > available + 0.5) { | ||
| cropElement.style.maxHeight = `${available}px`; |
There was a problem hiding this comment.
Measuring with the clamp cleared assumes the clamp doesn't change the space the layout gives the component. In a flex container where a sibling can also shrink, it does: the space each item loses is weighted by its flex basis, and the clamp lowers the component's basis. In a 500px-tall flex column with an image-crop (1000×3000 image) and a sibling with flex: 0 1 200px, the unclamped component gets 469px, so the image is clamped to 469px. With that smaller basis the component then gets 351px, and the image overflows it by 118px. It stays that way: at 250px the overflow is 98px, at 700px it is 119px.
|
|
||
| * Constrain the size of large images | ||
|
|
||
| The image is scaled down to fit the component's size, whether it is set with `height` or `max-height` (in any unit) or given by the layout. |
There was a problem hiding this comment.
This only holds when something limits the component's height. In a container that doesn't size its children, the component grows to the image's height, so nothing is clamped: with new Div(imageCrop) and div.setHeight("400px"), a 1000×4000 image renders at 4000px and overflows the Div. Percentages also need a parent with a defined height. Since HasSize was removed, setSizeFull() isn't available either.
| The image is scaled down to fit the component's size, whether it is set with `height` or `max-height` (in any unit) or given by the layout. | |
| The image is scaled down, keeping its aspect ratio, to fit the component's height when that height is limited: by a `height` or `max-height` set on the component, or by a flex layout such as `VerticalLayout` that sizes it. Percentages are resolved against the parent's height, so the parent needs a defined height. In a container that doesn't size its children, such as a `Div`, the component grows to the image's height; set a height on it, for instance `getStyle().set("height", "100%")`, to constrain the image. |
| * The image is scaled down, keeping its aspect ratio, to fit the component's width | ||
| * and height, whether they are set on the component (a {@code height} or a | ||
| * {@code max-height} in any unit) or given by the layout it is in. For instance, | ||
| * {@code getStyle().set("max-height", "100vh")} keeps a large image within the | ||
| * viewport. Images smaller than these limits are shown at their natural size. |
There was a problem hiding this comment.
Same as the README:
| * The image is scaled down, keeping its aspect ratio, to fit the component's width | |
| * and height, whether they are set on the component (a {@code height} or a | |
| * {@code max-height} in any unit) or given by the layout it is in. For instance, | |
| * {@code getStyle().set("max-height", "100vh")} keeps a large image within the | |
| * viewport. Images smaller than these limits are shown at their natural size. | |
| * The image is scaled down, keeping its aspect ratio, to fit the component's width | |
| * and height. The height is limited by a {@code height} or {@code max-height} set on | |
| * the component, or by a flex layout that sizes it, such as a {@code VerticalLayout}. | |
| * Percentages are resolved against the parent's height, so they only take effect when | |
| * the parent has a defined height. In a container that does not size its children, | |
| * such as a {@code Div}, the component grows to the image's height unless a height is | |
| * set on it. For instance, {@code getStyle().set("max-height", "100vh")} keeps a large | |
| * image within the viewport. Images smaller than these limits are shown at their | |
| * natural size. |
|
@javier-godoy Each round of measuring in JS fixes one layout case and exposes another (transforms during the Dialog animation, flex-shrink feedback), and the general case of fitting the image to whatever height the layout gives the component seems hard to get right. #8 and #12 only ask to keep large images within the viewport or a dialog. I'd suggest narrowing the scope: go back to the CSS-only approach from the first commit, drop |
As discussed in #12, this breaking change was deferred to the next major version, so it is included in 2.0.
The image now scales down to fit the component's max height (and the available width), keeping its aspect ratio and without upscaling small images. For instance,
getStyle().set("max-height", "70vh")keeps large images within a dialog.setImageFullHeight(breaking): usegetStyle().set("max-height", "100vh")insteadClose #12
Close #8