Sorry, not the best issue description…

The origin of this task emanates from https://www.drupal.org/node/2468445.

Because the thumbs contain a hard-coded wrapper

.media-thumbnail {
  width: 80px;
  height: 80px;
}

we also set max-width: 100%; and height: auto; properties on all the images that appear within that container, so that they do not accidentally bust out of the seams.

On our implementation, videos thumbnails in the media browser did not contain the prescribed panopoly-image-* class name on the img tag. So they busted out.

This patch seeks to provide a more bullet proof approach:

.media-thumbnail img {
  max-width: 100%;
  height: auto;
}

In revising panopoly-images.css I also eliminated duplicated properties, and made the labels more legible.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Andrew_Mallis created an issue. See original summary.

Andrew_Mallis’s picture

Andrew_Mallis’s picture

RobLoach’s picture

Status: Active » Reviewed & tested by the community
dsnopek’s picture

Status: Reviewed & tested by the community » Needs work

This mostly looks good! However...

+++ b/panopoly-images.css
@@ -100,3 +82,14 @@ img.panopoly-image-square {
+.media-thumbnail label {
+  line-height: 1.2em;
+  font-weight: normal;
+}

... I'm not crazy about this bit - things like line-height and font-weight are more for the theme, rather than a module which should be focused on sizing/structure, that sort of thing. And it doesn't seem like it's necessary to solve the problem in the issue summary.

I'm tempted to just remove that bit and then commit, but in case there is a good reason to put that in panopoly_images rather than a theme, I'd like to give you guys the opportunity to argue for it! So, marking to "Needs work" for now.

Andrew_Mallis’s picture

Debated putting that in there for similar reasons. Note here I am targeting only images provided by panopoly to make things better for these users.

Yes, this can be handled in the site's theme layer, but I am resetting styles set by style.css in seven – the default theme used in the Panopoly media browser:

body {
  color: #000;
  background: #fff;
  font: normal 81.3%/1.538em "Lucida Grande", "Lucida Sans Unicode", sans-serif;
}

So, this should legitimately be kicked upstream to the media module vs the theme layer.
Up to us to decide if we want faster action on on better UX. We do this in some other areas, I believe.

Here is the benefit:

demo before and after lineheight fix

Andrew_Mallis’s picture

FileSize
59.07 KB
dsnopek’s picture

Ok, so the idea is that it's styles from seven.css that the media browser depends on? Meaning that this fixes the media browser to work better with admin themes other than Seven?

dsnopek’s picture

Issue summary: View changes
FileSize
191.48 KB

Er, no, I was misunderstanding. This is changing away from some styles in Seven. So, the goal is to make it so that some two-line labels don't overlap the image when Seven is the admin theme?

Unfortunately, it doesn't work with all labels, and removing the bold makes the labels harder to read (at least for me):

Since this doesn't fix all labels (and actually, some two-line labels managed to fit before the patch), I think it'd be best to make a separate issue that tried to fix that issue comprehensively. I also think it'd be best to get some accessibility review on the color contrast and font size, because it definitely seems more difficult to read the label, but that's totally subjective at this point.

EDIT: Ok, I'm totally conflicted on what to do here. :-) Since the changes here are so small, it probably actually doesn't make sense to make a new issue... I'm going to have to think about this more.

Andrew_Mallis’s picture

Yes, when seven is the admin theme and filenames (labels) are long, they obscure the image.

Fonts in your screenshot are serif, but seven defines a sans serif font, so I am not sure we're comparing apples to apples.
See Comment #6.

Bold helps a lot with the serif fonts you show, but less so otherwise.

  • dsnopek committed 622ab38 on 7.x-1.x
    Update Panopoly Images for Issue #2573877 by Andrew_Mallis: tighten up...
dsnopek’s picture

Status: Needs work » Fixed

Fonts in your screenshot are serif, but seven defines a sans serif font, so I am not sure we're comparing apples to apples.

Ah, yeah, my screenshot is using Responsive Bartik as both the front-end and admin theme, which is the default for Panopoly on a fresh install.

Panopoly does pull in some of Seven's styles in the media browser because they're required for it to work correctly, which creates a theming mess, but we're kinda stuck with it without making a custom theme that knows how to style the media browser (rather than Responsive Bartik which is oblivious).

Since I feel like this makes things a little less readable with Panopoly's default settings, I'm going to omit the controversial bits of this patch, but get the main fix in. We can discuss merging the rest on another issue!

Thanks! :-)

Status: Fixed » Closed (fixed)

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