Issue summary updated as of comment #14

Problem/Motivation

  1. In Claros's media-grid the item's name sticks out of its expected place if the name of the media is shorter than width of the individual grid block. It exceeds both on the top and the bottom of the name space.
  2. Also, the uploaded media does not adhere to the standardised design, and floats above the media name space instead of being stuck to it.

Steps to reproduce

  1. Enable Media and Media Library modules.
  2. Goto /admin/content/media-grid
  3. Add media with short filename

Proposed resolution

  1. .media-library-item__attributes should be shrunk to address the media name space bleeding into either ends of the Y axis.
  2. Media element should be aligned to the top of its name space.

BEFORE patch

media before patch

AFTER patch

media after patch

CommentFileSizeAuthor
#33 3338309-30-test.gif2.63 MBnayana_mvr
#30 3338309-30-overhang.png4.81 KBckaotik
#30 3338309-30-hover-box.png10.24 KBckaotik
#22 AFTER-2.png129.33 KBakashdab
#22 BEFORE-2.png129.84 KBakashdab
#17 BEFORE.png78.64 KBakashdab
#17 AFTER.png80.07 KBakashdab
#11 3338309-after-patch.png752.51 KBnayana_mvr
#11 3338309-before-patch.png754.71 KBnayana_mvr
#10 interdiff-8_10.txt1.01 KBgauravvvv
#10 3338309-10.patch1.22 KBgauravvvv
#8 media-item-after.png15.74 KBZsuffa Dávid
#8 media-item-before.png15.76 KBZsuffa Dávid
#8 interdiff-3338309-3-8.txt785 bytesZsuffa Dávid
#8 3338309-8.patch1.19 KBZsuffa Dávid
#8 firma-media-item.png11.42 KBZsuffa Dávid
#7 Screen Shot 2023-02-02 at 15.03.09 1.png37.82 KBlauriii
#4 3338309-after-patch.png411.98 KBnayana_mvr
#4 3338309-before-patch.png441.6 KBnayana_mvr
#3 3338309-3.patch874 bytes_utsavsharma
#3 interdiff_2-3.txt440 bytes_utsavsharma
#2 3338309.patch434 bytesZsuffa Dávid
#2 media-grid-item-AFTER.png7.39 KBZsuffa Dávid
media-grid-item-BEFORE.png6.95 KBZsuffa Dávid

Issue fork drupal-3338309

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Zsuffa Dávid created an issue. See original summary.

Zsuffa Dávid’s picture

Zsuffa Dávid commented
Status: Active » Needs review
StatusFileSize
new media-grid-item-AFTER.png7.39 KB
new 3338309.patch434 bytes

I attached a possible patch to resolve the issue.

_utsavsharma’s picture

_utsavsharma commented
StatusFileSize
new interdiff_2-3.txt440 bytes
new 3338309-3.patch874 bytes

Tried to fix CCf for #2.

nayana_mvr’s picture

nayana_mvr commented
StatusFileSize
new 3338309-before-patch.png441.6 KB
new 3338309-after-patch.png411.98 KB

Verified the patch #3 and tested it on Drupal version 10.1.x. The issue is fixed after applying the patch and I have added the before and after screenshots for reference.

sonam.chaturvedi’s picture

sonam.chaturvedi commented

Verified and tested patch #3 on 10.1.x-dev. Patch applied successfully.

Test Steps:
1. Goto /admin/content/media-grid
2. Add media with short filename
3. Verify media-grid the item's name is higher than the space under the item preview
4. Apply patch #3
5. Verify media-grid the item's name is within the space

Test Result: media-grid the item's name is within the space under the item preview
Screenshot is same as #4

RTBC +1

Zsuffa Dávid’s picture

Zsuffa Dávid commented
Status: Needs review » Reviewed & tested by the community
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new Screen Shot 2023-02-02 at 15.03.09 1.png37.82 KB

Based on https://www.figma.com/file/VNkUIvfbcGr9Jmez3iRXJBjb/Media-widget-field?n..., it looks like there shouldn't be spacing between the name and the image preview. After this change, there would be some spacing:

Edit: The screenshot is before the patch is applied. However, the solution is to make the name less high, which essentially keeps the image in place and creates that spacing.

Zsuffa Dávid’s picture

Zsuffa Dávid commented
StatusFileSize
new firma-media-item.png11.42 KB
new 3338309-8.patch1.19 KB
new interdiff-3338309-3-8.txt785 bytes
new media-item-before.png15.76 KB
new media-item-after.png15.74 KB

In the figma design the media items don't contain any images.
figma media item
But based on your screenshot I made a new patch addressing the space issue.
media item before patch
media item after patch

Zsuffa Dávid’s picture

Zsuffa Dávid commented
Status: Needs work » Needs review
gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3338309-10.patch1.22 KB
new interdiff-8_10.txt1.01 KB

Fixed order/properties-order. Attached patch and interdiff for same. please review

nayana_mvr’s picture

nayana_mvr commented
StatusFileSize
new 3338309-before-patch.png754.71 KB
new 3338309-after-patch.png752.51 KB

Verified the patch #10 and tested it on Drupal version 10.1.x. Patch applied cleanly but for image with small height, there is still space between the name and the image preview. I have added the before and after screenshots for reference.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs Review Queue Initiative

Since this is a UI change screenshots should be added to the issue summary please.

Zsuffa Dávid’s picture

Zsuffa Dávid commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Issue summary proposed solution is to shrink item__attributes but I see we are editing the image. Proposed solution needs to match the patch solution.

Also seems to have caused a regression when you select an item in the media library widget a weird grey line appears on the side.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

akashdab’s picture

akashdab commented
Title: Media library grid item label sticks out » Media library grid item label sticks out, media alignment should adhere to positioning standards
Assigned: Unassigned » akashdab
Issue summary: View changes
Issue tags: -Needs issue summary update

Updated summary as of comment #14.
Positioning standards adopted from here.

akashdab’s picture

akashdab commented
Issue summary: View changes
StatusFileSize
new AFTER.png80.07 KB
new BEFORE.png78.64 KB

Adding before/after screenshots.

akashdab opened merge request !8547

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
akashdab’s picture

akashdab commented
Assigned: akashdab » Unassigned
smustgrave’s picture

smustgrave commented

Looking at the screenshots in the summary not sure this is an improvement? First image in the before definitely looks better.

akashdab’s picture

akashdab commented
Issue summary: View changes
StatusFileSize
new BEFORE-2.png129.84 KB
new AFTER-2.png129.33 KB

I think the before/after images attached previously did not highlight the issue and the solution, uploading a new set of images for better consideration.

akashdab’s picture

akashdab commented

I also think the addition of margin: 0.1rem; in media-library-item__attributes does not make much difference to the solution, rather it misaligns the title towards the top right corner. Video showing how.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

If part of the change isn't needed then that should be reverted. Good catch.

akashdab opened merge request !9369

akashdab’s picture

akashdab commented
Status: Needs work » Needs review

Made the suggested changes

smustgrave’s picture

smustgrave commented

Left 1 question on MR.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Believe this one is actually ready.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

Thanks for having the latest images available in the Issue summary.

Sorry, folks but there are two MRs here and there is no indication in the issue summary for which one to review. Not sure why there are two MR here.

@smustgrave, you say you left a comment in the MR but I can't find it in either one.

ckaotik’s picture

ckaotik
Primary language German
Location Berlin
commented
StatusFileSize
new 3338309-30-hover-box.png10.24 KB
new 3338309-30-overhang.png4.81 KB

Both merge requests are nearly identical, with MR !8547 including "margin" CSS rules in addition to the "padding" changed by MR !9369.

I've tried the patch in a setup using entity_browser, and have observed two problems still. Maybe this is due to entity_browser, but just wanted to make sure. If so, ignore what I said ;)

  1. Long labels that have are only fully shown when hovered still have a missmatched box size. This could be fixed with left: 0; right: 0;. Is this intentional?
    Hover effect on a library item showing a gap in the label box
  2. There is some kind of overhang for the hover effect. This is annoying when trying to hit something below, like an item in the next row or the submit button in an entity_browser.
    Mouse outside a library item but the item is still highlighted as being hovered
nayana_mvr’s picture

nayana_mvr commented

Regarding which MR to review:- As per #23,

..addition of margin: 0.1rem; in media-library-item__attributes does not make much difference to the solution, rather it misaligns the title towards the top right corner.

MR!9369 has the correct changes. So I'm hiding the branch of MR!8547 as i'm unable to close that MR.

nayana_mvr changed the visibility of the branch 3338309-media-library-grid to hidden.

nayana_mvr’s picture

nayana_mvr commented
StatusFileSize
new 3338309-30-test.gif2.63 MB

Regarding #30, I'm able to reproduce the point 1 (Please see the screen recording. This is after applying the current MR changes).

test

I think it is because of max-width calculation. For .media-library-item__attributes, a max-width: calc(100% - 10px); is used but I'm not sure why it’s added. Removing that or setting that to 100% fixes this issue. I can implement this change if someone can confirm about this solution.

As for point 2 in #30, I'm unable to reproduce that issue. Can be verified in the screen recording attached.

ckaotik’s picture

ckaotik
Primary language German
Location Berlin
commented

Thank you @nayana_mvr for the screen recording. Point 1 is indeed fixed by changing max-width: calc(100% - 0.625rem); to max-width: 100%. Though removing that style entirely would break the ellipsis effect.

I also checked again and I have a minor misalignment that only shows in Edge (Chrome-based browser), while Firefox displays fine. I suspect some rounding issues going on there.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

quietone’s picture

quietone commented
Status: Needs work » Postponed

The Claro theme was approved for removal in #3576460: [policy, no patch] Deprecate and remove Claro.

This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

The deprecation work is in #3576668: [meta] Tasks to deprecate Claro and the removal work in #3584638: [meta] Tasks to remove the Claro theme.