Editor External Media: improve perceived performance by rendering more loading placeholders #25776

jakejones1984 · 2018-06-26T22:02:44Z

Previously enough loading placeholders were rendered to cover the end of the row. However, users were reporting that the external media library loading was slow. After discussion design input suggested loading lots of placeholders than necessary would improve the perception of speed. This PR implements that feedback.

Steps to test:

On master:

Open Media modal within Editor
Choose "Google Photos"
Start scrolling down.
Notice how it feels slow

Now checkout this branch and repeat the process. Notice how much faster it feels now more placeholders are loaded at once.

Todo

Fix failing tests

Previously enough loading placeholders were rendered to cover the end of the row. However, users were reporting that the external media library loading was slow. After discussion design input suggested loading lots of placeholders than necessary would improve the perception of speed. This has been implemented. See * #22681 * #25581

matticbot · 2018-06-26T22:02:45Z

Test live: https://calypso.live/?branch=update/media-library-list-loading-placeholders

ehg · 2018-06-29T15:56:38Z

Could we add some screen capture GIFs here to show before & after?

jakejones1984 · 2018-06-30T16:36:19Z

Yes sorry @ehg, would have been good to illustrate the different visually.

Original

As you can see sometimes you only get a few loading squares showing up. This stops you from scrolling which provides the feeling of being "stuck". Also it's easy to missing the loading squares as there aren't many of them.

Revised

Now we added a number of loading squares as per design guidance from @drw158. This makes the entire scrolling experience feel a lot more fluid and less restrictive. Whilst the speed of loading is actually no faster it feels much quicker to the user.

Questions

@drw158 does this fit with your original proposal
Are there too many squares now?

davewhitley · 2018-07-03T15:33:59Z

Much much better! I don't think there are too many loading squares.

Some things I notices that may or may not be a separate issue:

Why is there a hard stop after just a couple of scrolls? The UI does not tell me why it's not loading any more even though I know there are more pictures.
The thumbnails provided by Google already have a play icon, so there is conflict with our own video icon.

jakejones1984 · 2018-07-03T16:20:06Z

Thanks @drw158. Let me have a look into those issues for you.

[Update]

The thumbnails provided by Google already have a play icon, so there is conflict with our own video icon.

I've added logic to remove the icon we're adding if the source if Google Photos

Why is there a hard stop after just a couple of scrolls? The UI does not tell me why it's not loading any more even though I know there are more pictures.

@drw158 Can you please use the Network section under devtools to debug this a bit?:

Open Chrome dev tools
Goto Network
Search for google_photos and it will bring up the requests going out to the API
Scroll to the bottom and click on the last 2-3 requests.
Send me the details or a GIF showing the details

I'm seeing mine bottom out at 237 photos. This looks like a bug because Picasa should allow at least 1000 photos.

@johngodley may be able to add some context here. Looking at the google_photos requests I see in the meta field of the responses (in order)

• next_page 280
• next_page 330
• next_page false

Video items coming from the Google Photos API have video icons automatically added as part of the thumbnail image. However, Calypso was then adding it’s own video icon over the top of this (as picked up in this PR #25776 (comment)). Fixes this by removing video icon if the source is Google Photos

johngodley · 2018-07-04T09:13:26Z

@jakejones1984 yes it appears to be maxing out. I've previously tested this up to 1000 so it definitely used to work that far. This leads me to think that a change has since occurred in Calypso or the WordPress API that is somehow limiting this. It's also possible that Picasa itself has changed. Certainly worth exploring while you are digging into this part of the code.

jakejones1984 · 2018-07-04T21:02:03Z

@johngodley I've done some testing and I can confirm Picasa is maxing out at the request where start-index is over 300

I get back a response from the Picasa API with absolutely no entry value at all. This means there are no photos being returned. This causes the WPCom API to (correctly) report that there are no more results. (side note: we need to update the UI to show a message when there are no more results - currently it just stops scrolling).

I suspect this issue is happening because we are defaulting to the recent folder path.

My theory is that the Picassa API has a limit on what they allow you paginate through within the "recent" folder. Moreover, the concept of "recent" suggests that there will be an arbitary limit on the number of photos Google classify as "recent".

To test this I made a request for a specific folder/album which I know has over 2000 photos in it. I passed a start-index of 400 and sure enough the Picasa API returned results! Therefore I believe our bug could be either:

A change in/regression of the Picasa API since launch which they haven't communicated
A bug that was always there from the start but which was never noticed until now (unlikely).

My recommendation is that we talk to Google to try and find out:

If they have changed the API so there's a hard limit on paginating the "recent" folder.
If it's possible to request "all" photos in any way.

...and yes I've noticed the tests are failing and I will fix them tomorrow!

@drw158 I've now added an clear notice (see screenshot below which shows the bottom half of the media list) that will appear when there are no more results to display. Obviously the core bug is still there (read above for explanation) but at least we're clearly notifying the user that we can't get any more results.

Previously when you reached the end of the available photos from the external media library nothing happened. Users had no feedback they’d reached the end of the list. Adds a Notice component which clearly states that there are no more photos to display. Also upgrades the notice that appears when you’ve reached the max results upper limit of 1000.

johngodley · 2018-07-05T10:01:08Z

A change in/regression of the Picasa API since launch

I've also confirmed this behaviour myself, and it seems the new max is 300. It used to be 1000, so this seems to be Google thing.

sidenote: we need to update the UI to show a message when there are no more results - currently it just stops scrolling
I've now added an clear notice (see screenshot below which shows the bottom half of the media list) that will appear when there are no more results to display

This is standard media library behaviour. The proposed changes here will affect the behaviour of the media library for everything, not just Google Photos. As such I think it should be done elsewhere.

Also to note that the notice will be shown even if you have a single photo, making it a bit overpowering.

the concept of "recent" suggests that there will be an arbitary limit on the number of photos Google classify as "recent"

The 'recent' term is our concept, and Google is just returning photos in reverse date order so there shouldn't be a limit on what recent means.

My recommendation is that we talk to Google

Existing restrictions/bugs in Picasa are going to remain - Google won't change anything outside of the new API.

My feeling is this adds more weight to using the new API. In the mean time we should reduce the current max limit check to 300, and then the existing Use the search button to access more photos. You can search for dates, locations, and things. message will appear - this is the current workaround.

…edia List" This reverts commit 55884c4.

jakejones1984 · 2018-07-07T23:40:47Z

@johngodley Thank you for your feedback. My responses below:

This is standard media library behaviour. The proposed changes here will affect the behaviour of the media library for everything, not just Google Photos. As such I think it should be done elsewhere.

Agreed. I've reverted this.

The 'recent' term is our concept, and Google is just returning photos in reverse date order so there shouldn't be a limit on what recent means.

I understand. However, if you don't request a specific Album from the API it appears to default to a kind of "recent" folder. I cannot confirm this but the following evidence leads me towards that conclusion

Hard limit of 300 odd photos when not specifying a folder (current functionality)
No limit observed when specifying a folder (please see this PR)

Existing restrictions/bugs in Picasa are going to remain - Google won't change anything outside of the new API.

I wasn't clear here. I meant, we should see if they are willing to confirm whether there is a limit when not requesting a specific folder/album. I feel it would be good to know for certain if possible.

My feeling is this adds more weight to using the new API.

Absolutely. For now however, please see my Album PR which provides a workaround to the hard limit.

In the mean time we should reduce the current max limit check to 300, and then the existing Use the search button to access more photos. You can search for dates, locations, and things. message will appear - this is the current workaround.

I'm going to propose keeping this PR related to the performance. @drw158 raised a couple of issues which I should have addressed elsewhere. We seem to agree that goal of "improving perceived performance" has been met and the other issues raised are now being solved in other PRs or workstreams.

Nonetheless, if you're happy for me to flip the value from 1000 to 300 I'm happy to do this. Please do let me know.

ehg · 2018-07-09T13:14:30Z

client/my-sites/media-library/list-item-video.jsx

@@ -23,11 +23,13 @@ export default class extends React.Component {
 		media: PropTypes.object,
 		maxImageWidth: PropTypes.number,
 		thumbnailType: PropTypes.string,
+		showIcon: PropTypes.bool,


We should move this showIcon fix into its own PR.

@ehg You're absolutely right. New PR created and changed reverted from this PR.

@ehg

Previously icon could not be disabled. In certain situations you might need to do this. For example, Google Photos automatically embeds a video icon on to the thumbnails of all videos coming through from it’s API. As a result we need to disable our icon so as not to double up. See #25776 (comment) Originally part of PR above but moved on advice from @ehg #25776 (review)

This reverts commit acb7d23.

alisterscott · 2018-07-24T05:44:57Z

Is this one close to being able to be merged?

jakejones1984 · 2018-07-30T23:00:31Z

Is this one close to being able to be merged?

@alisterscott Apologies for the delay here (been away). I believe it's nearly there. Just need @johngodley to confirm he's happy with 1000 => 300 change.

Nonetheless, if you're happy for me to flip the value from 1000 to 300 I'm happy to do this. Please do let me know.

@johngodley Are you able to confirm you're happy with the above? Much appreciated

johngodley · 2018-07-31T07:05:05Z

Are you able to confirm you're happy with the above? Much appreciated

Yep, let's move to 300.

jakejones1984 · 2018-07-31T17:28:58Z

@johngodley The hard limit is resolved in this PR so we might need to merge both PRs at the same time.

@alisterscott There's quite lot of PRs so apologies for any confusion.

getdave · 2019-01-02T10:56:54Z

This PR may be made redundant as part of the changes discussed at p4hfux-4s0-p2#comment-6708. Leaving it open for now to see if it's still valid post-changes.

stale · 2019-09-29T11:43:28Z

This issue has been marked as stale and will be closed in seven days. This happened because:

It has been inactive in the past 9 months.
It isn't a project or a milestone, and hasn’t been labeled `[Pri] Blocker`, `[Pri] High`, `[Status] Keep Open`, or `OSS Citizen`.

You can keep the issue open by adding a comment. If you do, please provide additional context and explain why you’d like it to remain open. You can also close the issue yourself — if you do, please add a brief explanation.

jakejones1984 requested review from ehg, davewhitley and designsimply June 26, 2018 22:02

Revert "Adds Notice to warn when no more photos to show in External M…

a1b8c9d

…edia List" This reverts commit 55884c4.

jakejones1984 added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Jul 8, 2018

ehg reviewed Jul 9, 2018

View reviewed changes

jakejones1984 mentioned this pull request Jul 10, 2018

Media Library - adds ability to show/hide icon for Video items #25969

Closed

Revert "Fixes Video icon from Google Photos video item thumbnails"

aad8705

This reverts commit acb7d23.

jakejones1984 mentioned this pull request Jul 17, 2018

External Media Google Photos: updates max photos hard limit to 300 #26129

Closed

jakejones1984 self-assigned this Jul 31, 2018

getdave assigned getdave and unassigned jakejones1984 Dec 12, 2018

stale bot added the [Status] Stale label Sep 29, 2019

stale bot closed this Oct 6, 2019

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 6, 2019

lancewillett deleted the update/media-library-list-loading-placeholders branch March 9, 2020 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor External Media: improve perceived performance by rendering more loading placeholders #25776

Editor External Media: improve perceived performance by rendering more loading placeholders #25776

jakejones1984 commented Jun 26, 2018 •

edited by getdave

Loading

matticbot commented Jun 26, 2018

ehg commented Jun 29, 2018 •

edited

Loading

jakejones1984 commented Jun 30, 2018

davewhitley commented Jul 3, 2018

jakejones1984 commented Jul 3, 2018 •

edited

Loading

johngodley commented Jul 4, 2018

jakejones1984 commented Jul 4, 2018 •

edited

Loading

johngodley commented Jul 5, 2018

jakejones1984 commented Jul 7, 2018

ehg Jul 9, 2018

jakejones1984 Jul 10, 2018 •

edited

Loading

alisterscott commented Jul 24, 2018

jakejones1984 commented Jul 30, 2018

johngodley commented Jul 31, 2018

jakejones1984 commented Jul 31, 2018

getdave commented Jan 2, 2019

stale bot commented Sep 29, 2019

Editor External Media: improve perceived performance by rendering more loading placeholders #25776

Editor External Media: improve perceived performance by rendering more loading placeholders #25776

Conversation

jakejones1984 commented Jun 26, 2018 • edited by getdave Loading

Todo

matticbot commented Jun 26, 2018

ehg commented Jun 29, 2018 • edited Loading

jakejones1984 commented Jun 30, 2018

Original

Revised

Questions

davewhitley commented Jul 3, 2018

jakejones1984 commented Jul 3, 2018 • edited Loading

johngodley commented Jul 4, 2018

jakejones1984 commented Jul 4, 2018 • edited Loading

johngodley commented Jul 5, 2018

jakejones1984 commented Jul 7, 2018

ehg Jul 9, 2018

Choose a reason for hiding this comment

jakejones1984 Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

alisterscott commented Jul 24, 2018

jakejones1984 commented Jul 30, 2018

johngodley commented Jul 31, 2018

jakejones1984 commented Jul 31, 2018

getdave commented Jan 2, 2019

stale bot commented Sep 29, 2019

jakejones1984 commented Jun 26, 2018 •

edited by getdave

Loading

ehg commented Jun 29, 2018 •

edited

Loading

jakejones1984 commented Jul 3, 2018 •

edited

Loading

jakejones1984 commented Jul 4, 2018 •

edited

Loading

jakejones1984 Jul 10, 2018 •

edited

Loading