Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Media Library widget has a weight field that allows specifying order of the attached media entities. The weight field doesn't have any effect when only single media can be attached to the field.
Proposed resolution
Remove weight field from the Media Library widget when only single media can be attached.
Remaining tasks
User interface changes
Correct cursor in media library and no weight field when selecting a single media library.
API changes
None
Data model changes
None
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#69 | after_patch.png | 257.54 KB | ambuj_gupta |
#67 | interdiff_64-67.txt | 1.23 KB | himanshu_sindhwani |
#67 | 3102402-67.patch | 7.31 KB | himanshu_sindhwani |
#64 | interdiff_55-64.txt | 3.71 KB | himanshu_sindhwani |
#64 | 3102402-64.patch | 6.83 KB | himanshu_sindhwani |
Comments
Comment #2
Devipriya Rajamanickam CreditAttribution: Devipriya Rajamanickam commentedKindly provide screenshots for better understanding.
Comment #3
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedKindly follow a new patch.
Comment #4
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedKindly follow a new patch
Comment #7
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedkindly follow a new patch.
Comment #9
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedkindly review a new patch
Comment #10
lauriiiIt seems like there are some coding standard violations. You can run
composer run phpcs core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
to see those.Comment #11
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedCoding standard violations are resolved in new patch
Comment #14
idebr CreditAttribution: idebr at iO commentedAttached patch updates the test coverage.
Comment #16
sathish.redcrackle CreditAttribution: sathish.redcrackle at Red Crackle commentedI have reviewed the patch and attached the screenshots before and after patch applied.
Error screenshot
After patch applied
Comment #17
sathish.redcrackle CreditAttribution: sathish.redcrackle at Red Crackle commentedComment #18
idebr CreditAttribution: idebr at iO commentedThe patch in #14 still has a failing test.
Comment #19
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedkindly follow a new patch.
Comment #21
sathish.redcrackle CreditAttribution: sathish.redcrackle at Red Crackle commentedFixed 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.
Comment #22
lauriiiThe weight field is still visible when JavaScript is disabled.
Comment #23
sathish.redcrackle CreditAttribution: sathish.redcrackle at Red Crackle commentedI have fixed the issue mentioned on comment #22 and attached new patch.
Comment #24
phenaproximaThanks, @sathish.redcrackle! Will you also add test coverage to ensure that the weight field doesn't show up when there is only one item?
Comment #25
sathish.redcrackle CreditAttribution: sathish.redcrackle at Red Crackle commentedI added the test to check hidden weight field and attached patch.
Comment #26
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedComment #27
snehalgaikwad CreditAttribution: snehalgaikwad at QED42 for Drupal India Association commentedPatch in #25 is working perfectly, now 'Show media item weights' doesn't appear if only one media can be attached to the field.
Comment #28
phenaproximaThis 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!
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?
Nit: The word "inputs" should have an apostrophe after it: "...the toggle for the weight inputs' visibility..."
This seems unnecessarily complex; I think the following would suffice instead:
$assert_session->fieldNotExists('Weight', $wrapper);
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).
This also seems overly complicated. Why not just
$assert_session->fieldNotExists('Weight')
?Comment #29
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commentedHere is the new patch to fix points according comment #28, Kindly review.
Comment #30
hash6 CreditAttribution: hash6 at QED42 commentedComment #31
hash6 CreditAttribution: hash6 at QED42 commentedThanks @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
Comment #32
alexpott#31 contains core/modules/layout_builder/tests/src/FunctionalJavascript/BlockLocatorTrait.php which is unrelated to this patch.
Comment #33
hash6 CreditAttribution: hash6 at QED42 commentedComment #34
hash6 CreditAttribution: hash6 at QED42 commentedSorry, @alexpott Updated the patch. Please review it again.
Comment #35
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedComment #36
Rangaswini CreditAttribution: Rangaswini at QED42 for Drupal India Association commentedComment #37
Rangaswini CreditAttribution: Rangaswini at QED42 for Drupal India Association commentedComment #38
mayurgajar CreditAttribution: mayurgajar commentedComment #39
mayurgajar CreditAttribution: mayurgajar at QED42 for Drupal India Association commentedHi @ hash6,
patch #34 apply cleanly LGTM +1 RTBC .
Thanks..!!!
Comment #40
mayurgajar CreditAttribution: mayurgajar at QED42 for Drupal India Association commentedComment #41
lauriiiWe should also remove
cursor: move
from.media-library-item__preview
in this scenario because the element isn't draggable.Comment #42
phenaproximaThanks for this patch! I have a few concerns about the tests, though.
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.)The comment here is a little strangely worded; how about "Ensure that the styling doesn't accidentally render the weight field unusuable" instead?
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. :)
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.
Comment #43
mayurgajar CreditAttribution: mayurgajar at QED42 for Drupal India Association commentedComment #44
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commentedHii, I have update patch wrt comment #41 and #42, kindly review new patch.
Comment #46
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedI've re-rolled patch for 9.1
Comment #48
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedComment #49
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch to 9.1.x.
Comment #50
ambuj_gupta CreditAttribution: ambuj_gupta at QED42 commentedComment #51
ambuj_gupta CreditAttribution: ambuj_gupta at QED42 commentedSteps 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.
Comment #52
alexpottIs this always correct? I think we still need this when there are more that 1 .media-library-item__preview on the page.
Comment #53
shaktikHi @alexpott,
yes, I think still need, updated on this patch kindly check.
Comment #54
alexpott@shaktik the issue is that
cursor: move;
is needed when there is more than one item and not when there is only one item.Comment #55
shaktikHi @alexpott,
updated
cursor:move;
and weight_access to access variable, kindly check.Comment #56
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedComment #57
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedVerified 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) -
Comment #58
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedComment #59
DeepaliJ CreditAttribution: DeepaliJ at QED42 for Drupal India Association commentedComment #60
DeepaliJ CreditAttribution: DeepaliJ at QED42 for Drupal India Association commentedVerified and tested on local by applying patch #55. Looks good to me.
Can be moved to RTBC.
Refer #57 for steps and screenshots.
Comment #61
alexpott$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.
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...
That CSS will ensure that the move cursor only appears when there are multiple items.
We can use this in seven too I think.
Comment #62
shaktikComment #63
himanshu_sindhwani CreditAttribution: himanshu_sindhwani at Srijan | A Material+ Company for Drupal India Association commentedwill work on the feedbacks mentioned n #61
Comment #64
himanshu_sindhwani CreditAttribution: himanshu_sindhwani at Srijan | A Material+ Company for Drupal India Association commentedUpdated the patch according to #61.
Comment #65
himanshu_sindhwani CreditAttribution: himanshu_sindhwani at Srijan | A Material+ Company for Drupal India Association commentedComment #66
alexpottAs 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.2Comment #67
himanshu_sindhwani CreditAttribution: himanshu_sindhwani at Srijan | A Material+ Company for Drupal India Association commentedUpdated.
Comment #68
ambuj_gupta CreditAttribution: ambuj_gupta at QED42 commentedComment #69
ambuj_gupta CreditAttribution: ambuj_gupta at QED42 commentedTested and verified by the apply patch #67. And it's working as expected.
Comment #70
alexpottFixing spelling mistake on commit.
Fixed coding standard too.
Backported to 9.0.x as a bugfix.
Comment #74
jrbThese 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.