Hi,

When using portrait image on a large screen, the image is scaled to fit the editing form width, it often lead to a vertical overflow with scroll bar.
It's specially true in modal windows (entity browser).

Maybe an option could be added to force the full image to fit in the browser window ?

CommentFileSizeAuthor
#12 2866715.png154.62 KBO'Briat
#9 allow_crop_zone_to_fit-2866715-9.patch3.21 KBwoprrr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

O'Briat created an issue. See original summary.

woprrr’s picture

woprrr CreditAttribution: woprrr as a volunteer and at NeoLynk commented

Hi @O’Briat,

Your feature describes are already covered by the option “crop thumbnail”. I explain my point of view, with the possibility to use an image style instead of display images to crop directly have two advantages. First Image Widget Crop can calculate the expansion or reduction of image only when one size is fixed (with or height). In “crop thumbnail” case by default the fixed value is “with” and ImageStyle apply a scale to no destroy image to crop and reproduce selected zone into original. You can change it or define your proper style to using in the “crop thumbnail” option and take the height fix only to provide you needed.

What you think about?

O'Briat’s picture

O'Briat CreditAttribution: O'Briat at Capgemini for E.voyageurs Technologies, SNCF commented

I perfectly understand the image style “crop thumbnail” point (as you explain it to me at the DDD), but your module css enlarge the image to 100% width :

/*image_widget_crop/css/image_widget_crop.css*/
.crop-preview-wrapper__preview-image {
  display: block;
  width: 100%;
  height: auto;
  margin-bottom: 1em;
}

So a portrait/skyscraper image will be taller than the screen, thus adding vertical scrollbar, even if you “crop thumbnail” limit its height to a small heigh (100px for example).

I try to override the css (without losing image ratio) :

div .crop-preview-wrapper__preview-image {
  width:auto;
  max-height: 80vh;
}

But it triggers a cropper.js error on the following case : a content creation + first crop only :
TypeError: o is undefined cropper.min.js:10:20026

The cropping works fine, but the results is wrong : the first crop is incorrect, the following ones are ok. If you edit the first crop again and save everything is fine.

Note:
About the “crop thumbnail”, tell me if I'm wrong but you can use a scale effect with both width and height set, since the effect respect the image ratio ?

woprrr’s picture

woprrr CreditAttribution: woprrr as a volunteer and at NeoLynk commented
Related issues: +#2852141: Clarify all documentation

But it triggers a cropper.js error on the following case : a content creation + first crop only :
TypeError: o is undefined cropper.min.js:10:20026

The cropping works fine, but the results is wrong : the first crop is incorrect, the following ones are ok. If you edit the first crop again and save everything is fine.

That is already identify and due to have "Always expand crop area" option activated @see #2862709: Crop first area are wrong when "Always expand crop area" is set to "YES" for a full feedback + help us to solve it. This issue represent last blockers before have a stable release 2.0.

I perfectly understand the image style “crop thumbnail” point (as you explain it to me at the DDD), but your module css enlarge the image to 100% width :

It's true, during sprint with MD system guys we have decide to move all calculations class in PHP to Js and it's possible to have a calculation system a little different as my original think. Let's me try it and provide a more feedback about it when I have small time to see this (actually I'm a little busy). We need too have a better documentation to avoid all problems like your questions and this issue can be a good element of this documentation effort.

O'Briat’s picture

O'Briat CreditAttribution: O'Briat at Capgemini for E.voyageurs Technologies, SNCF commented

So if issue #2862709 is fixed, my hack should work and could be turned into a feature to allow image to fit the screen height.

I'll try to have a look at #2862709, you could contact me over irc if you need me to done some specific works

woprrr’s picture

woprrr CreditAttribution: woprrr as a volunteer and at NeoLynk commented

So if issue #2862709 is fixed, my hack should work and could be turned into a feature to allow image to fit the screen height.

Yes, for problems lie to "expand" option. The better way to not have this problem is always close detail area (that is better IMHO). When we have large BO with few fields/groups having a crop element automatically open aren't a good UX improvment. The Open/Close detail does be open only if we need to changes/define crop manual onto images and that is not always our cases (80% of time) the crop by center take good results.

About the “crop thumbnail”, tell me if I'm wrong but you can use a scale effect with both width and height set, since the effect respect the image ratio ?

No, you can't. If we decide to define your sizes you have few chances to destroy the original aspect ratio of image. You do alway have minimum one size automatic for scale correctly if you not this is called "resize". Scale & crop effect can check if your sizes destroy the aspect ratio and try to solve it but I'm not sure this is a good idea.

Specifically in Entity_browser / modal case we need to add an event to tell Modal js about the box have upcrease because if you open the element "crop" you change the heigth.

woprrr’s picture

woprrr CreditAttribution: woprrr as a volunteer and at NeoLynk commented
Status: Active » Postponed (maintainer needs more info)

All does works now onto 2.x-dev branch. Can you confirm please ?

O'Briat’s picture

O'Briat CreditAttribution: O'Briat at Capgemini for E.voyageurs Technologies, SNCF commented

Scaling will maintain the aspect-ratio of the original image. If only a single dimension is specified, the other dimension will be calculated.

I made the test, if you provide both height and width, the image will be resize without any ratio modification. This effect just made sure that none of the resulting dimensions will be be greater than ones you filled.

So it should be safe to use it that way for "Crop preview image style".

I totally understand that any image style effects that change ratio should be avoided, like resize for example:

Resizing will make images an exact set of dimensions. This may cause images to be stretched or shrunk disproportionately.
woprrr’s picture

woprrr CreditAttribution: woprrr as a volunteer and at NeoLynk commented
Status: Postponed (maintainer needs more info) » Needs review
FileSize
allow_crop_zone_to_fit-2866715-9.patch3.21 KB

With this we can use an image style only based on width or height. The restriction of sizes are resolved by 2.x branches that part are migrated on JS part and not by php. Can you test your case now please ?

Status: Needs review » Needs work

The last submitted patch, 9: allow_crop_zone_to_fit-2866715-9.patch, failed testing. View results

woprrr’s picture

woprrr CreditAttribution: woprrr as a volunteer and at NeoLynk commented
Status: Needs work » Needs review
O'Briat’s picture

O'Briat CreditAttribution: O'Briat at Capgemini for E.voyageurs Technologies, SNCF commented
Status: Needs review » Needs work
FileSize
2866715.png154.62 KB

Hi,
Sorry it doesn't seems to works :

  1. Add image style: scale, height 100px
  2. Use it as a "crop preview"in your image field widget settings
  3. Add a image
  4. The image file as a 100px height but it is scale by inline css added by js, see screenshot

As I said in comment 3, css (and possibly the crop js) need to be modified.

woprrr’s picture

woprrr CreditAttribution: woprrr as a volunteer and at NeoLynk commented
Status: Needs work » Needs review

I show you a simple test to how fit the height of crop zone ==> https://youtu.be/PYw7fOCNU2c All works good to me. The css you show me in elements aren't provided by IWC but more by modal I guess.

sallakane’s picture

sallakane CreditAttribution: sallakane at NeoLynk for Carrefour commented

Hi @all,

I tested in a modal and this patch seems to work well on my side.
Salla

woprrr’s picture

woprrr CreditAttribution: woprrr as a volunteer and at NeoLynk commented
Status: Needs review » Reviewed & tested by the community

At this point all seems to works as excepted. We can consider this RTBC be Neolynk support team. @O'Briat if you have time can you confirm or you'r free to switch this issue status if that not works to your case or create a new issue more specific.

  • woprrr committed 3eebbf7 on 8.x-2.x 
    Issue #2866715 by woprrr, O'Briat, sallakane: Allow Crop zone to fit in...
woprrr’s picture

woprrr CreditAttribution: woprrr as a volunteer and at NeoLynk commented
Status: Reviewed & tested by the community » Fixed

Merged Thanks all ;)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.