-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bug] Image cropper completely redesigned #576
base: 1.x
Are you sure you want to change the base?
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
Hi @manuelkleinert the actual bugfix branch is 1.4. Could you please change the base branch to 1.4? Thanks |
@manuelkleinert Thanks a lot for re-basing. The PR looks really good, just one small thing. There is a minimum width when resizing the selection. Could you please fix this? Thanks |
@robertSt7 We have intentionally implemented this so that the image cannot have a lower resolution. Alternatively, we could allow this and simply colour it red with a warning notice. |
@manuelkleinert That's a good idea, please implement that and could you please also re-base the PR to 1.x? Thanks a lot |
@robertSt7 The image cropper now allows smaller selections but displays a warning |
@manuelkleinert Thank you for that. Could you please remove the aspect of ratio enforcement too and display just a warning? Also please change the base branch to 1.x, since this is an improvement. Thanks a lot |
@robertSt7 The ratio of the images should be maintained, as they should be cropped in proportion and not distorted. For us, it makes no sense to remove this. |
@manuelkleinert When you use it in DataObjects you can actually crop your image with every proportion and the rest is filled up with a white background. If we merge this, this function will not be available anymore. And what happens if you have different proportions in media queries? |
Fixed a bug in the cropper, if it has no height, it is defined as 100
Quality Gate passedIssues Measures |
I have also fixed a bug with the default height when it is not defined. @robertSt7 With the data objects you can freely choose the area, since no thumbnails are defined there or am I misunderstanding something? If the section can be larger than the image, I would have to make the window larger than the image. |
@manuelkleinert Sorry for being so picky about this one. We really like your ideas and your work 😍 . We see an issue though with the hard limit of the aspect ratio. I see two ways we could tackle:
|
@manuelkleinert any thoughts from your side? |
@fashxp Sorry I don't have time to work on it right now, only in 2-3 weeks. |
what do you mean by that? |
@manuelkleinert friendly 🏓 |
Could you make me some printscreens where exactly the problem might appear in the object? |
If I determine the max height and max width of all the different sizes, we could use these for the image cutout, right? And I could also deactivate the aspect on cover. |
What do you mean with that?
I'm not sure, if it is worth the complexity (from implementation perspective as well as from user perspective when aspect ratio is fixed, when not). |
Quality Gate passedIssues Measures |
Hmm, the original image could be much bigger, like 5000x2000px. So the restriction for 800x300px is not applicable, and even the aspect ratio 8:3 is not really of an value here, right? Let's try not to complicate things too much 😄 |
Image cropper completely redesigned with consideration of thumbnail sizes and several bug fixes.
In order for the dimensions of the thumbnail to be taken into account in the cropper, the thumbnail must also be defined in the Image Editable