Infinite scroll in Media #562

merapi · 2020-03-12T23:08:57Z

This adds infinite scroll to the Media gallery (#23).

Next steps (can be included in this PR):

Lazy load images - right now we have loading="lazy" which works in Chrome/Edge, soon FF, we can utilize IntersectionObserver to build our own solution.
This solution loads as many pages as needed to fill the whole viewport. For example with per_page=2 and rootMargin: 400px it will load ~10 pages to fill the viewport and then load one row (2 images) as you start scrolling down - not sure if it's useful but it's there "for free" (for per_page=100 it will load 100 one time - as expected in AC).
Keyboard support - be able to see all images with arrows.
UX, loading states.

assets/src/edit-story/app/api/apiProvider.js

assets/src/edit-story/components/library/panes/media/mediaPane.js

assets/src/edit-story/utils/useIntersectionEffect.js

spacedmonkey · 2020-03-13T14:15:17Z

This branch needs to be rebased.

spacedmonkey · 2020-03-13T14:15:44Z

I like that after upload, the view reset and scrolls to the top 👍

spacedmonkey

One small tweak and a question, but otherwise, we are looking good.

merapi · 2020-03-13T15:07:13Z

One small tweak and a question, but otherwise, we are looking good.

Done, I think we can merge it and work from there.

googlebot · 2020-03-13T15:17:52Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

spacedmonkey · 2020-03-13T15:18:31Z

@googlebot I consent.

googlebot · 2020-03-13T15:18:49Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

github-actions · 2020-03-13T15:20:29Z

Size Change: +933 B (0%)

Total Size: 414 kB

Filename	Size	Change
`assets/js/edit-story.js`	361 kB	+933 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`assets/css/edit-story.css`	225 B	0 B
`assets/css/stories-dashboard.css`	165 B	0 B
`assets/js/stories-dashboard.js`	53.4 kB	0 B

_{compressed-size-action}

miina · 2020-03-13T16:30:59Z

Is this fixing #23? Or #247?

(Edit: just saw that should be 23 based on the branch name, mind adding it to description as well?)

miina · 2020-03-13T16:33:29Z

@merapi I'm getting a console error and the editor is breaking:

Wanted to test before reviewing, does it work for you?

spacedmonkey · 2020-03-13T17:22:00Z

I am not seeing the console errors.

spacedmonkey · 2020-03-13T17:23:33Z

Please use the keyword fixes #23 to denote which issue this is linked.

Also make sure to ticket to code review and assign some other devs.

assets/src/edit-story/utils/useIntersectionEffect.js

assets/src/edit-story/components/library/panes/media/mediaPane.js

assets/src/edit-story/app/media/useMediaReducer/reducer.js

assets/src/edit-story/app/media/mediaProvider.js

assets/src/edit-story/app/media/useMediaReducer/reducer.js

spacedmonkey · 2020-03-16T11:41:31Z

This PR needs to be merge into master. Much has changed in media.

assets/src/edit-story/components/library/panes/media/mediaPane.js

assets/src/edit-story/app/api/apiProvider.js

assets/src/edit-story/app/media/mediaProvider.js

dvoytenko · 2020-03-16T19:19:22Z

assets/src/edit-story/app/media/useMediaReducer/reducer.js

        mediaType,
      };
    }

+    case types.SET_PAGE: {


Pls clarify: what does the SET_PAGE do comparing to the FETCH_MEDIA_SUCCESS? Is this request-vs-response data? That'd seem dangerous to conflate them into a single property, no?

We now just have SET_NEXT_PAGE.

assets/src/edit-story/components/library/panes/media/mediaPane.js

dvoytenko · 2020-03-16T19:32:14Z

assets/src/edit-story/components/library/panes/media/mediaPane.js

+                {resources
+                  .filter((_, index) => isEven(index))
+                  .map((resource, i) => (
+                    <MediaElement


My first reaction to this is that, most like loading=lazy will no longer suffice here. Both because it's not well supported, and also because we'd likely want to add more logic to visible vs not visible state. It's definitely fine to do this over several pull requests, of course. To highlight here: much of the performance tradeoffs will come from the <video> where we have more granularity: we can load the video, or just metadata, etc.

And looking at the current MediaElement, this would also improve the structure of that component(s). For instance, we can split it into two tiers: MediaElementWrapper that takes care of the sizing, focus, hover, etc. And a type-specific Preview component that's only rendered when the element is "visible" and can use system resources as it sees fit. This would also make a good place to deal with "poster backfill" and any such issue.

dvoytenko · 2020-03-16T19:35:18Z

assets/src/edit-story/components/library/panes/media/mediaPane.js

+      if (!hasMore) return;
+      if (!entry.isIntersecting) return;
+
+      setPage({ page: page + 1 });


Looking more into the use here, I don't see why we need both page and setPage. I think if we instead introduce loadNextPage() method in our context, it'd solve all of these nits: we won't be leaking bulk of the API and the meaning of this will be very clear.

Agree, but I would call it setNextPage because it doesn't load the next page by itself.

ES6 import is failing (repo cause): https://travis-ci.com/github/google/web-stories-wp/jobs/297848579

spacedmonkey

This looks good. 👍

Old review

merapi requested review from barklund, dvoytenko, pbakaus, spacedmonkey and swissspidy as code owners March 12, 2020 23:08

googlebot added the cla: yes label Mar 12, 2020

spacedmonkey reviewed Mar 13, 2020

View reviewed changes

assets/src/edit-story/app/api/apiProvider.js Outdated Show resolved Hide resolved

spacedmonkey reviewed Mar 13, 2020

View reviewed changes

assets/src/edit-story/components/library/panes/media/mediaPane.js Outdated Show resolved Hide resolved

spacedmonkey reviewed Mar 13, 2020

View reviewed changes

assets/src/edit-story/utils/useIntersectionEffect.js Outdated Show resolved Hide resolved

spacedmonkey suggested changes Mar 13, 2020

View reviewed changes

merapi requested a review from miina as a code owner March 13, 2020 15:05

spacedmonkey force-pushed the feature/23-infinite-scroll branch from 16d32af to 2b5c8a0 Compare March 13, 2020 15:12

googlebot added cla: no and removed cla: yes labels Mar 13, 2020

googlebot added cla: yes and removed cla: no labels Mar 13, 2020

merapi linked an issue Mar 13, 2020 that may be closed by this pull request

Media Library - Infinite Scroll #23

Closed

spacedmonkey reviewed Mar 16, 2020

View reviewed changes

assets/src/edit-story/utils/useIntersectionEffect.js Outdated Show resolved Hide resolved

miina reviewed Mar 16, 2020

View reviewed changes

assets/src/edit-story/app/media/useMediaReducer/reducer.js Show resolved Hide resolved

dvoytenko suggested changes Mar 16, 2020

View reviewed changes

assets/src/edit-story/components/library/panes/media/mediaPane.js Outdated Show resolved Hide resolved

assets/src/edit-story/components/library/panes/media/mediaPane.js Show resolved Hide resolved

dvoytenko previously requested changes Mar 16, 2020

View reviewed changes

merapi added 9 commits March 17, 2020 00:03

Infinite scroll (paginated) implemented

b6fc78d

Switch to CommonJS because it’s working, fix JSDoc

35439b1

ES6 import is failing (repo cause): https://travis-ci.com/github/google/web-stories-wp/jobs/297848579

Remove polyfill 🎉

eb42006

Better use of initial state

6940e6d

Make function name more generic.

8cdb00e

Pass node directly

f957d35

Don’t reset search term when switching between types

32d05ea

Fix null node

7f8bc7b

Apply comments: naming, rootMargin

5d7fb0d

merapi force-pushed the feature/23-infinite-scroll branch from 16d3ef5 to 5d7fb0d Compare March 16, 2020 23:11

merapi requested a review from dvoytenko March 16, 2020 23:13

spacedmonkey approved these changes Mar 17, 2020

View reviewed changes

spacedmonkey merged commit 53c4622 into master Mar 17, 2020

spacedmonkey deleted the feature/23-infinite-scroll branch March 17, 2020 12:54

swissspidy added the Type: Enhancement New feature or improvement of an existing feature label Mar 19, 2020

merapi mentioned this pull request Jun 9, 2020

Media: Fix for loading past page 1 #2314

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infinite scroll in Media #562

Infinite scroll in Media #562

merapi commented Mar 12, 2020 •

edited

Loading

spacedmonkey commented Mar 13, 2020

spacedmonkey commented Mar 13, 2020

spacedmonkey left a comment

merapi commented Mar 13, 2020

googlebot commented Mar 13, 2020

spacedmonkey commented Mar 13, 2020

googlebot commented Mar 13, 2020

github-actions bot commented Mar 13, 2020 •

edited

Loading

miina commented Mar 13, 2020 •

edited

Loading

miina commented Mar 13, 2020

spacedmonkey commented Mar 13, 2020

spacedmonkey commented Mar 13, 2020

spacedmonkey commented Mar 16, 2020

dvoytenko Mar 16, 2020

merapi Mar 16, 2020

dvoytenko Mar 16, 2020

dvoytenko Mar 16, 2020

merapi Mar 16, 2020

spacedmonkey left a comment

Infinite scroll in Media #562

Infinite scroll in Media #562

Conversation

merapi commented Mar 12, 2020 • edited Loading

spacedmonkey commented Mar 13, 2020

spacedmonkey commented Mar 13, 2020

spacedmonkey left a comment

Choose a reason for hiding this comment

merapi commented Mar 13, 2020

googlebot commented Mar 13, 2020

spacedmonkey commented Mar 13, 2020

googlebot commented Mar 13, 2020

github-actions bot commented Mar 13, 2020 • edited Loading

miina commented Mar 13, 2020 • edited Loading

miina commented Mar 13, 2020

spacedmonkey commented Mar 13, 2020

spacedmonkey commented Mar 13, 2020

spacedmonkey commented Mar 16, 2020

dvoytenko Mar 16, 2020

Choose a reason for hiding this comment

merapi Mar 16, 2020

Choose a reason for hiding this comment

dvoytenko Mar 16, 2020

Choose a reason for hiding this comment

dvoytenko Mar 16, 2020

Choose a reason for hiding this comment

merapi Mar 16, 2020

Choose a reason for hiding this comment

spacedmonkey left a comment

Choose a reason for hiding this comment

merapi commented Mar 12, 2020 •

edited

Loading

github-actions bot commented Mar 13, 2020 •

edited

Loading

miina commented Mar 13, 2020 •

edited

Loading