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.
Comment | File | Size | Author |
---|---|---|---|
#9 | Selection_123.png | 191.48 KB | dsnopek |
#7 | 2573877-demo.png | 59.07 KB | Andrew_Mallis |
#3 | panopoly_images-media-browser-tighter-2573877.patch | 1.29 KB | Andrew_Mallis |
Comments
Comment #2
Andrew_Mallis CreditAttribution: Andrew_Mallis at Kalamuna commentedComment #3
Andrew_Mallis CreditAttribution: Andrew_Mallis at Kalamuna commentedComment #4
RobLoachComment #5
dsnopekThis mostly looks good! However...
... 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.
Comment #6
Andrew_Mallis CreditAttribution: Andrew_Mallis at Kalamuna commentedDebated 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:
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:
Comment #7
Andrew_Mallis CreditAttribution: Andrew_Mallis at Kalamuna commentedComment #8
dsnopekOk, 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?
Comment #9
dsnopekEr, 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.
Comment #10
Andrew_Mallis CreditAttribution: Andrew_Mallis at Kalamuna commentedYes, 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.
Comment #12
dsnopekAh, 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! :-)