Remove weight field from Media Library widget when only single media can be attached

Kindly provide screenshots for better understanding.

Kindly follow a new patch.

Kindly follow a new patch

kindly follow a new patch.

kindly review a new patch

Coding standard violations are resolved in new patch

Attached patch updates the test coverage.

I have reviewed the patch and attached the screenshots before and after patch applied.

Error screenshot

After patch applied

The patch in #14 still has a failing test.

kindly follow a new patch.

Fixed the test issue in Comment #14 and attached the modified patch file.

The test failed because the media popup is not triggered so the second media is not attached to the form. Also attached the interdiff file.

I have fixed the issue mentioned on comment #22 and attached new patch.

Thanks, @sathish.redcrackle! Will you also add test coverage to ensure that the weight field doesn't show up when there is only one item?

I added the test to check hidden weight field and attached patch.

Patch in #25 is working perfectly, now 'Show media item weights' doesn't appear if only one media can be attached to the field.

This looks very good to me, except for some minor points and requests for simplification/clarification. I'm removing the "Needs tests" tag, since we certainly have adequate coverage now. Thank you!

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -354,6 +354,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +        '#access' => count($referenced_entities) > 1,
            '#attributes' => [
   ...
              '#title' => $this->t('Weight'),
   +          '#access' => count($referenced_entities) > 1,
   

   The #access expression is repeated, which increases its chance of breaking due to future refactoring. Can we put it in a variable and re-use that?

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
   @@ -191,11 +191,14 @@ public function testWidget() {
   +    // The toggle for weight inputs visibility should not be available when
   +    // the field contains a single item.
   

   Nit: The word "inputs" should have an apostrophe after it: "...the toggle for the weight inputs' visibility..."

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
   @@ -191,11 +191,14 @@ public function testWidget() {
   +    $weights = $this
   +      ->xpath('//div[(contains(@style,"display: none"))]//input[(contains(@class,"js-media-library-item-weight"))]');
   +    $this
   +      ->assertEquals(0, count($weights), "Weight field should be shown if there is one item.");
   

   This seems unnecessarily complex; I think the following would suffice instead:

   $assert_session->fieldNotExists('Weight', $wrapper);

4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
   @@ -213,6 +216,13 @@ public function testWidget() {
   +    $assert_session->fieldExists('Weight', $wrapper)->click();
   

   We should add a comment explaining why we click on the Weight field (it's to ensure that the styling doesn't accidentally render it unclickable).

5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/WidgetAnonymousTest.php
   @@ -60,12 +61,28 @@ public function testWidgetAnonymous() {
   +    $assert_session->elementNotExists('css', '.js-media-library-widget-toggle-weight');
   +    $weights = $this
   +      ->xpath('//div[(contains(@style,"display: none"))]//input[(contains(@class,"js-media-library-item-weight"))]');
   +    $this
   +      ->assertEquals(0, count($weights), "Weight field should be shown if there is one item.");
   

   This also seems overly complicated. Why not just $assert_session->fieldNotExists('Weight')?

Here is the new patch to fix points according comment #28, Kindly review.

Thanks @swatichouhan012 for the patch ,we need couple of changes
As mentioned by @phenaproxima
#1 #access expression , I have added the complete $weight_access = count($referenced_entities) > 1;
#2 "weight input's" changed to "weight inputs'"
Rest #3, #4 and #5 seems perfect.

After patch weight field & link disappears

#31 contains core/modules/layout_builder/tests/src/FunctionalJavascript/BlockLocatorTrait.php which is unrelated to this patch.

Sorry, @alexpott Updated the patch. Please review it again.

Hi @ hash6,

patch #34 apply cleanly LGTM +1 RTBC .

Thanks..!!!

Thanks for this patch! I have a few concerns about the tests, though.

-    // Assert that we can toggle the visibility of the weight inputs.
+    // The toggle for weight inputs' visibility should not be available when
+    // the field contains a single item.
     $wrapper = $assert_session->elementExists('css', '.field--name-field-twin-media');
-    $wrapper->pressButton('Show media item weights');
-    $assert_session->fieldExists('Weight', $wrapper)->click();
-    $wrapper->pressButton('Hide media item weights');
+    $assert_session->fieldNotExists('Weight', $wrapper);

I don't understand this change. We are asserting that the "weight" field does not exist, which is correct...but shouldn't we also assert that the toggle itself is not present? (Something like $assert_session->elementNotExists('named', ['button', 'Show media item weights'], $wrapper) should do it.)

+    // It's to ensure that the styling doesn't accidentally render it
+    // unclickable.
+    $assert_session->fieldExists('Weight', $wrapper)->click();
+    $wrapper->pressButton('Hide media item weights');

The comment here is a little strangely worded; how about "Ensure that the styling doesn't accidentally render the weight field unusuable" instead?

     // Ensure that the selection completed successfully.
     $this->waitForText('Dog');
+    $assert_session->fieldNotExists('Weight');

If possible, maybe we should also get the field wrapper element here and assert that it, specifically, does not contain a "Weight" field. Otherwise, this is going to assert that there is no field by that name on the entire page, which is a side effect we probably don't want. :)

     $this->submitForm([
       'title[0][value]' => 'My page',
       'field_unlimited_media[selection][0][weight]' => '0',
+      'field_unlimited_media[selection][1][weight]' => '1',
     ], 'Save');

I'm not sure this change is strictly necessary, but it does help to ensure that the weight fields are working as intended. So I'm fine with keeping it, just thought I'd point it out here.

Hii, I have update patch wrt comment #41 and #42, kindly review new patch.

I've re-rolled patch for 9.1

Rerolled patch to 9.1.x.

Steps to Test
1. Enable Media module (with media library).
2. Apply patch #49.
2. Go to "/admin/structure/types/manage/page/fields"
3. Add a media field to the basic page content type.
- Set the limit to 5.
- Set the media type to "Image"
4. Go to "/node/add/page"
5. Click on the Add Media button. And upload a single image.
6. Insert the Image and check the weight field.
7. Edit the above created node and upload multiple images and check the weight field.

Results:
Verified after applying patch #49. and it's working as expected.

1. When there is a single image, the weight field is hidden.
2. When there is more than one image, the weight field is coming.

+++ b/core/themes/claro/css/theme/media-library.css
@@ -478,7 +478,6 @@
 .media-library-item__preview {
   padding-bottom: 34px;
-  cursor: move;
 }

+++ b/core/themes/claro/css/theme/media-library.pcss.css
@@ -440,7 +440,6 @@
 .media-library-item__preview {
   padding-bottom: 34px;
-  cursor: move;
 }

+++ b/core/themes/seven/css/theme/media-library.css
@@ -456,7 +456,6 @@
 .media-library-item__preview {
   padding-bottom: 34px;
-  cursor: move;
 }

Is this always correct? I think we still need this when there are more that 1 .media-library-item__preview on the page.

Hi @alexpott,

yes, I think still need, updated on this patch kindly check.

diff --git a/core/themes/claro/css/theme/media-library.css b/core/themes/claro/css/theme/media-library.css
index 1e78d16d6d..b41f78eac0 100644
--- a/core/themes/claro/css/theme/media-library.css
+++ b/core/themes/claro/css/theme/media-library.css
@@ -478,6 +478,7 @@

 .media-library-item__preview {
   padding-bottom: 34px;
+  cursor: move;
 }

 .media-library-item__status {
diff --git a/core/themes/claro/css/theme/media-library.pcss.css b/core/themes/claro/css/theme/media-library.pcss.css
index f2d74c38e1..78970b4dcd 100644
--- a/core/themes/claro/css/theme/media-library.pcss.css
+++ b/core/themes/claro/css/theme/media-library.pcss.css
@@ -440,6 +440,7 @@
 /* Media library entity view display styles. */
 .media-library-item__preview {
   padding-bottom: 34px;
+  cursor: move;
 }

 .media-library-item__status {
diff --git a/core/themes/seven/css/theme/media-library.css b/core/themes/seven/css/theme/media-library.css
index e6c7400360..80217a64bc 100644
--- a/core/themes/seven/css/theme/media-library.css
+++ b/core/themes/seven/css/theme/media-library.css
@@ -456,6 +456,7 @@
 /* Media library entity view display styles. */
 .media-library-item__preview {
   padding-bottom: 34px;
+  cursor: move;
 }

@shaktik the issue is that cursor: move; is needed when there is more than one item and not when there is only one item.

Hi @alexpott,

updated cursor:move; and weight_access to access variable, kindly check.

Verified and tested by applying the patch #55.It looks good to me.Can be moved to RTBC.

Steps to test -
1. Go to the admin site.
2. Go to admin/modules.
3. Enable Media module (with media library).
4. Go to "/admin/structure/types/manage/page/fields"
5. Add a media field to the basic page content type.
- Set the limit to 5.
- Set the media type to "Image"
6. Go to "/node/add/page"
7. Click on the Add Media button. And upload a single image.
8. Insert the Image and check the weight field.
9. Edit the above created node and upload multiple images and check the weight field.

Before Patch (Single) -

Before Patch (Multiple) -

After Patch (Single) -

After Patch (Multiple) -

Verified and tested on local by applying patch #55. Looks good to me.
Can be moved to RTBC.
Refer #57 for steps and screenshots.

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -342,10 +342,12 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      $access = count($referenced_entities) > 1;
   

   $access is not a great name here. $weight_access was better... but $multiple_items would be even more descriptive. Also it should be defined outside of the if.

2. +++ b/core/themes/claro/css/theme/media-library.css
   @@ -478,9 +478,12 @@
    .media-library-item__preview {
      padding-bottom: 34px;
   -  cursor: move;
    }
    
   +.move-cursor .media-library-item__preview {
   +  cursor: move;
   + }
   

   So the original cursor move causes another problem - which this patch happens to fix. It results in a move cursor on the media library dialog but the items can not be moved. So that's a good thing.

   However it is possible to add the move cursor without adding another class...

   .field--widget-media-library-widget .media-library-item__preview {
     cursor: move;
   }
   
   .field--widget-media-library-widget .js-media-library-item:only-child .media-library-item__preview {
     cursor: inherit;
   }
   

   That CSS will ensure that the move cursor only appears when there are multiple items.

   We can use this in seven too I think.

will work on the feedbacks mentioned n #61

Updated the patch according to #61.

+++ b/core/themes/claro/css/theme/media-library.css
@@ -481,6 +481,10 @@
   cursor: move;
 }

+++ b/core/themes/claro/css/theme/media-library.pcss.css
@@ -443,6 +443,10 @@
   cursor: move;
 }

+++ b/core/themes/seven/css/theme/media-library.css
@@ -459,6 +459,10 @@
   cursor: move;
 }

As per these cursor: move; also need to be more specific as this only applies to the .field--widget-media-library-widget - atm this CSS breaks the cursor on the media item listing when it is in the media library modal. See #61.2

Updated.

Tested and verified by the apply patch #67. And it's working as expected.

diff --git a/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
index eef4a64d13..17e72a0af2 100644
--- a/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
+++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
@@ -216,7 +216,7 @@ public function testWidget() {
     $wrapper = $assert_session->elementExists('css', '.field--name-field-twin-media');
     $wrapper->pressButton('Show media item weights');
     // Ensure that the styling doesn't accidentally render the weight field
-    // unusuable.
+    // unusable.
     $assert_session->fieldExists('Weight', $wrapper)->click();
     $wrapper->pressButton('Hide media item weights');
 

Fixing spelling mistake on commit.

PHPCS: core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php passed

FILE: ...ary/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 194 | ERROR | [x] No space found before comment text; expected "// /
     |       |     The toggle for weight inputs' visibility should
     |       |     not be available when" but found "/// The toggle
     |       |     for weight inputs' visibility should not be
     |       |     available when"
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Fixed coding standard too.

Backported to 9.0.x as a bugfix.

These changes caused the JavaScript bug described in #3327234: Dragging single media thumbnail on edit page causes JavaScript error because the expected weight field was no longer in the markup and available to the drag-and-drop code.