Reader: Cut down image transfer significantly #11400
Conversation
Also, only try to preload 5 images max. This cuts down on the number of images we need to transfer to load Reader pages and sigificantly speeds up how we choose what image to show.
blowery
added
[Feature] Reader
blowery
changed the title
|
|
||
| const FeaturedImage = ( { imageUri, href, children, onClick } ) => { | ||
| if ( imageUri === undefined ) { | ||
| return null; | ||
| } | ||
|
|
||
| const featuredImageStyle = { | ||
| backgroundImage: 'url(' + cssSafeUrl( imageUri ) + ')', | ||
| backgroundImage: 'url(' + cssSafeUrl( resizeImageUrl( imageUri, { w: 250 } ) ) + ')', |
There was a problem hiding this comment.
Worth sticking the 250 into a const and explaining why we're using that value?
|
|
||
| function FeaturedImage( { image, href } ) { | ||
| const uri = resizeImageUrl( safeImageUrl( image.uri ), { w: 385 } ) |
There was a problem hiding this comment.
Worth sticking the 385 into a const and explaining why we're using that value?
| return urlOrPromise; | ||
| } | ||
| return promiseForImage( imageForURL( urlOrPromise ) ); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Worth a comment block here explaining the general approach and how promises are used?
There was a problem hiding this comment.
yeah, it got a little weird.
There was a problem hiding this comment.
ah, and it's not needed anymore! I pulled this check down.
|
Functionality looks good 👍 |
| naturalWidth: knownDimensions.width, | ||
| naturalHeight: knownDimensions.height | ||
| }; | ||
| } | ||
| imagesToCheck.push( image.src ); |
There was a problem hiding this comment.
do we still want to check images that we already know?
There was a problem hiding this comment.
nevermind. you skip loading them later
| @@ -43,7 +43,12 @@ function promiseForImage( image ) { | |||
| } ); | |||
| } | |||
|
|
|||
| const promiseForURL = flow( imageForURL, promiseForImage ); | |||
| const promiseForURL = function( urlOrPromise ) { | |||
| if ( urlOrPromise instanceof Promise ) { | |||
There was a problem hiding this comment.
are we passing in a promise anywhere?
There was a problem hiding this comment.
yup, when we already know the size. I was trying to do the minimal thing to the image loading pipeline here, so I represented a "known" image as an already resolved promise, resolving it with a duck-typed image object. Not super clean, but it slips through the flow nicely.
There was a problem hiding this comment.
actually... this is done elsewhere now. Ripping it out.
| @@ -14,7 +14,7 @@ import cssSafeUrl from 'lib/css-safe-url'; | |||
| import { isFeaturedImageInContent } from 'lib/post-normalizer/utils'; | |||
| import ReaderPostCardExcerpt from './excerpt'; | |||
|
|
|||
| const GALLERY_ITEM_THUMBNAIL_WIDTH = 420; | |||
| const READER_CONTENT_WIDTH = 800; | |||
There was a problem hiding this comment.
Should this be 800 or 720?
I think it would be better if we reused (and/or updated) all of the constants from client/state/reader/posts/normalization-rules.js
There was a problem hiding this comment.
That one confused me too. We use 720, but cards can definitely be 800 with the redesign.
|
Wow. While on my personal Following stream the difference isn't so stark, when I measured any photo-heavy tag stream it was massive. I'm talking about half the network traffic. To test I cleared cache and went to
|
samouri
added
[Status] Needs Author Reply
and removed
[Status] Needs Review
blowery
added
[Status] Needs Review
samouri
added
[Status] Ready to Merge
and removed
[Status] Needs Review
The primary thing here is teaching
waitForImagesToLoadhow to detect existing dimensions on images. This prevents the majority of preload requests for size for WordPress hosted sites. In addition, it now only tries to preload 5 images, max.This PR also teaches the post card, gallery card, and recommended post card to load smaller images by default.
This cuts down on the number of images we need to transfer to load Reader pages and significantly speeds up how we choose what image to show.
To test, pull up the Following stream and scroll. Make sure the network tab is open. You should see significantly less network traffic, mostly for images.