Hide the crop type tabs when only one crop type is available

Agree, this is major for overall UX. Most of the time there will only be one crop type.

Patch #2 doens't work with latest dev version

1. +++ b/src/Element/ImageCrop.php
   @@ -85,10 +85,12 @@ class ImageCrop extends FormElement {
   +        '#type' => $has_single_croptype ? 'container' : 'details',
   

   That should not usefull. Because details are a sort of extend of container that same...

2. +++ b/src/Element/ImageCrop.php
   @@ -112,7 +114,7 @@ class ImageCrop extends FormElement {
   +        '#type' => $has_single_croptype ? 'container' : 'vertical_tabs',
   

   I'm not sure about that. In js we are based a lot onto vertical tabs element to fired events. We need to test it more.

I flag this issue prior to solve during Media Sprints DDD Sevilla. We need to talk more about UX of IWC...

Fixing event tag :)

I rewrote the patch, based on latest 2.x-dev.

Hi @Cauliflower, Thank to your help ! and sorry for the delay actually I'm pretty busy with other contribution projects.

But I review/test it fast (today).

Looks good

Hii @Cauliflower,

There is my feedback about your patch.

1. +++ b/src/Element/ImageCrop.php
   @@ -84,11 +84,13 @@ class ImageCrop extends FormElement {
   +      $has_single_croptype = count($crop_type_list) == 1;
   

   I think we can extract this kind of test into public function like $this->hasSingleCropType() to reduce general complexity of these BIG BIG (too big and too complex) method.

2. +++ b/src/Element/ImageCrop.php
   @@ -84,11 +84,13 @@ class ImageCrop extends FormElement {
   +        '#type' => $has_single_croptype ? 'container' : 'details',
   

   I have some doubt about that part because actually we need to inform users when crop are apply or not with "Crop image (cropping applied)" flag. With this details element the UX seems more clear IMHO.

3. +++ b/src/Element/ImageCrop.php
   @@ -84,11 +84,13 @@ class ImageCrop extends FormElement {
   +          'class' => ['image-data__crop-wrapper', 'image-data__crop-wrapper-croptype-' . ($has_single_croptype ? 'single' : 'multiple')],
   

   Where these class are used ? I think that sound better if we can set an attribute instead of class to sibling these element in js ?

@Pls Have you some idea to increase the UX of ImageWidgetCrop ? We are totally open to purpose and feedback about IWC interface.

Patch #11 is working for me, but not for an image field with multiple values.

I'm come back to this issue ! @StryKaizer I have some problems by replace "details" element to container but why not play with '#open' Form Attributes to show/hide the crop area ? In 2.x whe also have a settings to control that "Always expand crop area: Yes/No" we can extend that to automatically set to Yes when widget have only one item ? What to you think about this ?

I have a different approach to this problem. Rather than trying to test to see if there is just one crop type, let's allow the site builder to choose whether to use tabs on a per field instance. This has the advantage of simplifying the code, but more importantly, it means that adding a crop type does not also alter the form output. This also has the added flexibility of allowing more than one crop type to be stacked one on top of another (without the vertical tabs), if that is the desired result.

Attached is a first pass at achieving this result. I have added a "Wrapper Type" field, with options of "Vertical Tabs" and "Stacked Divs".

I altered the patch in #17 to make sure it applies on the 2.2 release.

Updated the patch from #18 to work with the latest version 2.3.0 and 2.x-dev

Whichever of this or #3116312: Hide vertical tabs when there is only one image style available in committed first, the other will have to be refactored.

I've testes both patches (#20 from this issue and #7 from #3116312) and they both apply and work fine. But I'd prefer the one from this issue because gives administrators more control and makes the change more predictable.

I think there are two things that should be addressed before I'd feel good about committing this.

1. This really needs automated test coverage. One approach would be to modify ImageWidgetCropTest such that there's a new method which adds an additional crop type and makes it available to the image field created in setUp(), then uploads a new image and checks that multiple tabs are visible. Additionally, we could alter an existing test case (which only has one crop type) to assert that there are no tabs.
2. Skimming the patch, I'm not sure I understand how the implementation in there aligns with the title and summary of this issue. I see a lot of stuff about changing the wrapper type, which is all well and good, but I don't understand how that affects whether to display the tabs or not, depending on how many crop types there are. Am I missing something?

I think I prefer the, "less is more" approach here. Why do we need to make the type of rendering configurable. Wouldn't the approach in #3203154: Hide the crop type name when only one crop type is available be more simple? If someone really needs true configurability, then they can element info alter and change the element to their own custom implementation that lets it dance and have unicorns. But for the primary use case, swapping between details and a div are enough.