Make the lightbox work with image-deferrer (bug 1030159) #526
Conversation
| src to show the rocket image in order to try to avoid having | ||
| coordinates completely out of whack when the image is replaced by | ||
| the real one */ | ||
| background-position: center center; |
Previews seem to stop loading after I close the lightbox. Images are not loading in other lightboxes I open either. |
|
Will look into that, I had not tested previews in search at all :/ |
|
Ahah, so the problem lies with scrolling: when you visit the search results page, you scroll down, and when you show the lightbox, that scroll amount is still used in the calculations even though the lightbox is fixed and doesn't care about the vertical scroll. Working on a fix. |
|
Should be fixed now. I'm about to write some tests, in the meantime, r? @ngokevin |
|
image deferring on the search page seems broken, images mostly aren't loading. Probably a change in the offset logic? |
|
It's working for me ? Something that's still broken is the switch between expanded/compact styles, I'll fix that, in the meantime when you switch, reload the page. I also need to double-check that we aren't loading the images in compact mode. |
|
Doesn't work for me in either mode, and even after I scroll. After I switch + reload, it works. But on a fresh search:
|
|
Ok, adding that to my testcase list, I'll look into it tomorrow. |
|
All cases should be working for me now. I've added some tests, but it's far from covering everything - it's very hard to write good tests here, because a) we depend on stuff not being loaded (without knowing what's an appropriate amount of time to wait to make sure it has not been loaded) b) we're using a very old version of casper with an abstraction layer that makes it super difficult to use advanced casper functionalities. |
|
yeah, our testing framework needs freshening. It's something I'd like to get to sometime. I'm not too worried about full coverage here though, I don't remember a time when our frontend tests caught any regressions. |
| @@ -74,8 +58,10 @@ define('image-deferrer', ['underscore', 'urls', 'z'], function(_, urls, z) { | |||
|
|
|||
| var scrollListener = function(e) { | |||
| if (!$images) { | |||
| return; | |||
| // console.info('No images to load, ignoring...'); | |||
There was a problem hiding this comment.
and all the other debug statements
|
r+squash, works great |
|
FYI: not merged yet because the last time we pushed the flipsnap upgrade, it caused bug 1041838. I'm trying to come up with a way to detect this issue locally and find a suitable workaround. |
|
Let's hold this up until we get feed merged in. I did a lot of stuff with image deferring logic (support background-images) and styles. |
|
@diox if you need any help with converting these tests to the new casper setup let me know. |
|
@ngokevin based on your comment about "hold this up until we get feed merged in". It's merged what should we do now? |
|
Now this patch needs to be updated with merge conflicts and flipsnap upgrade. |
|
Yep, see the TODO I've left in the description. |
|
Going to close this in the meantime. |
https://bugzilla.mozilla.org/show_bug.cgi?id=1030159
Losing the swipe w/ snap made me sad, so here is my attempt to keep flipsnap, making image deferring work with it.
TODO: