Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
If a Spotlight widget has long text in the title or the description, the display breaks at the smallest mobile widths. The title text sticks out above the top of the image, and the pager buttons overlay and obscure part of the title.
Would it be worth it to set a maximum character length on the title and description? Or is there a way to manage the display so that it doesn't look like this? See screenshot, taken at 320px width.
Comment | File | Size | Author |
---|---|---|---|
#14 | interdiff.txt | 554 bytes | dsnopek |
#14 | panopoly_widgets-small-spotlight-2623318-14.patch | 1.4 KB | dsnopek |
panopoly-spotlight-long-text.png | 380.28 KB | cboyden |
Comments
Comment #2
dsnopekHere's a first pass at this. I tested it under Chrome and Firefox, and it seems to work.
It makes me a little nervous, though... I'll post specifically which bits worry me in the next comment (so I can use Dreditor).
Comment #3
dsnopekI'm not sure why this -6px is necessary, and it's a pretty specific magic number.
When changing to "overflow: hidden" this 6px gap just appears under the label. Why?
This could break a custom theme that styles the spotlight without completely removing the panopoly-widgets-spotlight.css
I guess the same could be said of the other changes, except this one is working on 'p' tags which are commonly styled in custom themes (whereas the rest are 'div' tags).
Comment #4
dsnopekAlso, since the CSS for spotlight labels is already pretty complex we should really test this under a couple IE versions as well.
Comment #5
dsnopekI tested on Browserstack with IE11, IE10, IE9 and IE8.
In IE9-11, it looks perfect.
In IE8, it passable-ish -- there's actually issues with the Play/Pause buttons and Responsive Bartik, so this is definitely the least of our problems. The main difference in IE8 is that the magic gap mentioned in #3 is less then 6px (maybe 4-5px) so the label overlaps the description slightly.
This could probably use much, much more testing under more browsers and devices...
Comment #6
dsnopekI tested under the following mobile browser on Browserstack (per it's suggested list to get the most coverage):
I needed some changes to prevent the label from overlapping below the spotlight, and some artifacts of the 2nd line of text in the label appearing on the Galaxy S5.
However, on the iPad Air 2, it doesn't have the magic 6px gap at all, so the label overlaps the description. :-/ If we could figure out why that gap appears on the desktop and remove the need for the -6px margin, that would be ideal! That said, it's the only browser (mobile or desktop) that I've tested so far that doesn't have the gap, so if it's really the only, only one, that might be acceptable...
Comment #7
dsnopekEr, forgot to attach the patch with changes for the testing in #6!
Comment #8
dsnopekHere's a new patch! It should work for more font sizes without necessarily needing special theming (at least to work - not necessarily to look good ;-)). It also solves (finally!) the mysterious -6px gap issue, which makes me feel a little more comfortable about this patch in general.
I've tested it in IE13, IE11, IE9 (sort of arbitrary selection), Chrome, Firefox, Opera 36 and Safari 9.
Next up is another round of mobile testing!
Comment #9
dsnopekOk, I retested the mobile devices from #6 and they all worked great! The problem I had previously with the iPad Air 2 is no longer present. :-)
Comment #10
dsnopekHere's an updated version that sets a min-height so that there is always room for the label and description, even if the image is broken or not loaded yet. Of course, we shouldn't have broken images, and try to make sure the images are loading fast, but now that we're clipping the title and description to the size of the widget, it started looking kinda crazy when those things happened. :-) This isn't perfect but it's an improvement.
EDIT: Here's a Travis build with this patch: https://travis-ci.org/panopoly/panopoly/builds/126643703EDIT 2: That build used the patch from #8: https://travis-ci.org/panopoly/panopoly/builds/126715089
Comment #11
dsnopekBah! The last patch didn't merge with the 7.x-1.x branch. Here's a better version - the interdiff is still correct.
Comment #12
dsnopekIn order to help debug, I started #2717277: Upload screenshots to imgur on failed tests
Here's a build which is using that patch with this patch:
https://travis-ci.org/dsnopek/panopoly/builds/127204291
Comment #13
dsnopekSo, for some reason this totally breaks our tests. :-/
Here's the image from the failed tests:
http://i.imgur.com/ti80UDb.png
It shows that the image or the title and description are visible. I suspect (but don't know for sure) that the image might not have been loading during the tests, but these CSS changes make it so that the title/description don't appear when the image doesn't load. In any case, we have to get this passing in order to merge it!
Here's a new travis build just in case this fixed itself in the meantime: https://travis-ci.org/panopoly/panopoly/builds/188953584
Comment #14
dsnopekOk, here's another attempt at doing what I tried to do in #10/#11 that will hopefully work on Travis.
EDIT: Here's the Travis build: https://travis-ci.org/panopoly/panopoly/builds/195991711
Comment #16
dsnopekAha! Finally passed testing on Travis. Committing!