Repository navigation
feat!: constrain the image to the component's max height - #42
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
| Layer / File(s) | Summary |
|---|---|
Component sizing API src/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.java |
ImageCrop implements HasSize and documents how the image scales to the available width and maximum height. The setImageFullHeight(Boolean) method was removed. |
Sizing styles and usage examples src/main/resources/META-INF/frontend/styles/image-crop-styles.css, README.md, src/test/java/com/flowingcode/vaadin/addons/imagecrop/UploadImageCropDemo.java |
The crop element displays as inline-block with a maximum width of 100%, and the .ReactCrop wrapper inherits max-height. The README and demo set the component’s maximum height to 70vh. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Feature · Severity of issue fixed: Medium
Merge Risk: 
ea843
-
The crop image is constrained by the configured maximum height, so the documented sizing behavior is supported. No actionable issue remains before merge.
Pre-merge checks: 
-
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 "feat!: constrain the image to the component's max height" matches the main change in the changeset. The PR makes ImageCrop implement HasSize, scales large images to fit the maximum height a… 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: Title check
-
Explanation
The title "feat!: constrain the image to the component's max height" matches the main change in the changeset. The PR makes ImageCrop implement HasSize, scales large images to fit the maximum height and available width, and removes setImageFullHeight. The "!" marker correctly flags the breaking change. The title names one aspect of the change, the height constraint, and omits the width scaling and the removal of setImageFullHeight. That omission is acceptable because the title does not need to cover every detail.
-
Finishing Touches
-
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
Tip
🎯 Customize your risk assessments
-
Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.
✅ Automatic approvals, with your rules
-
Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.
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.
ℹ️ Recent review info
-
⚙️ Run configuration
-
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
2949e623-cc99-4309-b9db-1742d64295e1
📒 Files selected for processing: 4
-
README.mdsrc/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.javasrc/main/resources/META-INF/frontend/styles/image-crop-styles.csssrc/test/java/com/flowingcode/vaadin/addons/imagecrop/UploadImageCropDemo.java
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
ℹ️ Recent review info
-
⚙️ Run configuration
-
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
2949e623-cc99-4309-b9db-1742d64295e1
📒 Files selected for processing: 4
-
README.mdsrc/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.javasrc/main/resources/META-INF/frontend/styles/image-crop-styles.csssrc/test/java/com/flowingcode/vaadin/addons/imagecrop/UploadImageCropDemo.java
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
-
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.
There was a problem hiding this comment.
Not applicable anymore: as discussed in the PR comments, the scope was narrowed to CSS only in ebe2ae4, so the image is no longer measured.
| 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.
There was a problem hiding this comment.
Not applicable anymore: the JS measuring was removed in ebe2ae4; flex sizing is documented as not supported.
|
|
||
| * 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. |
There was a problem hiding this comment.
Updated in ebe2ae4: the README now says that only absolute and viewport units for max-height are supported.
| * 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. |
There was a problem hiding this comment.
Updated in ebe2ae4: the Javadoc now says that only absolute and viewport units for max-height are supported.
|
@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 |
I agree, its better to have a simple solution that works. |
8895cf0 to
ebe2ae4
Compare
|
@scardanzan Thanks! Done in ebe2ae4: the JS measuring was removed and the image is fitted to |
ebe2ae4 to
c095293
Compare
|
github-project-automation
Bot
moved this from In Progress
to Pending release
in Flowing Code Addons
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