Fix Safari bug: Missing images #57
Conversation
goldenapples
left a comment
goldenapples
left a comment
There was a problem hiding this comment.
Looks like a good improvement. I think there probably needs to be a more graceful fallback in place for IE users, where we can't use intersection observers - right now it looks like images will just be missing on IE.
| }; | ||
|
|
||
| loadLazily = images; | ||
| scrollHandler(); |
There was a problem hiding this comment.
You've removed the call to this function - can the function definition above be removed?
There was a problem hiding this comment.
Thanks! As I'm working from the vendor folder, the IDE isn't warning me about these things
There was a problem hiding this comment.
You can symlink packages into the vendor folder from elsewhere if it means your IDE isn't doing any processing on files under that directory.
assets/src/gaussholder.js
Outdated
| visibleImages.forEach( ( { target } ) => loadOriginal( target ) ); | ||
| }, options); | ||
|
|
||
| Array.from( images ).forEach( ( img ) => { |
There was a problem hiding this comment.
Array.from (and IntersectionObserver) aren't available in IE. What kind of fallback should we provide so that IE users don't just lose all the images on the site?
There was a problem hiding this comment.
I've separated the code into a few more modules. The main file in src/gaussholder.js will choose a handler based on the Intersection API availability now.
assets/src/gaussholder.js
Outdated
|
|
||
| Array.from( images ).forEach( ( img ) => { | ||
| imagesObserver.observe( img ); | ||
| handleElement( img ); |
There was a problem hiding this comment.
I notice that this handle element routine is no longer waiting until the image is loaded. What kinds of cases was the onload callback earlier set up to handle? Do we need to account for potentially needing to wait until the blank spacer gif loads?
There was a problem hiding this comment.
It's funny that you mention this because the plugin wasn't working at all when the bundle was the production one and that's because the development was bringing the source maps and it seems that those ms used to load the sourcemaps are crucial to initialize the images. The production bundle was initialized too early.
I've done some changes for this. The Gaussholder function is now called when the DOM is loaded instead and it seems to be working like a charm :)
|
@goldenapples Excellent observations, thank you so much. I've done a few more changes:
|
roborourke
left a comment
roborourke
left a comment
There was a problem hiding this comment.
Looks really good for the most part but some bits seem to have gotten lost along the way. See the review comments for specifics :)
| @@ -0,0 +1,20 @@ | |||
| const intersectionHandler = function( images, loadImageCallback ) { | |||
| const options = { | |||
| rootMargin: '1200px' | |||
There was a problem hiding this comment.
This seems very specific. Could we get some explanatory comments in this file? I have no idea what I'm looking at really.
There was a problem hiding this comment.
@roborourke This is a threshold to detect the image (more or less, is quite more complex than that). We are already using 1200 threshold in the scroll handler so I left the same quantity. I added a comment anyway
| } | ||
| loadLazily = next; | ||
| if (loadLazily.length < 1) { | ||
| window.removeEventListener('scroll', scrollHandler); |
There was a problem hiding this comment.
The scroll event listener is no longer added to the window object anywhere that I can see. Is that a bug or intentional? If intentional this bit can be removed as it's not doing anything right now.
There was a problem hiding this comment.
@roborourke You're right and I've fixed it by refactoring some parts of the code, the images weren't loading if they were not passing the threshold. I've added a throttle function to simplify things too.
roborourke
left a comment
roborourke
left a comment
There was a problem hiding this comment.
Minor code style changes but otherwise I'm happy with this. I haven't tested this myself so hoping this al looks good your end @igmoweb !
|
@roborourke As ESLint wasn't set up, I opted to create an extra PR to do so and lint all the JS files. Here it is: #58 |
|
Will give it another full test tomorrow and merge if everything looks fine |
ESlint configuration
3 participants
Under some circumstances, Safari was failing to display some images due to a particularity in the browser that was preventing onload event to be triggered on cached images once the Gaussholder script had already started
After some testing, it seems that webpack bundling + the use of IntersectionObserver API did the trick. It also improves performance when scrolling the page as calculations are very easy to do now.
The PR introduces the use of IntersectionObserver (props to @goldenapples who worked on the issue already here #48 and because he gave me the idea to use the new API)