Media: Fix for loading past page 1 #2314

diegovar · 2020-06-09T02:42:15Z

Summary

Swap useIntersectionObserver to receive a properly updated ref.

Introduced in #2069

Relevant Technical Choices

Uses an extra ref.

To-do

None.

User-facing changes

Fixes the media pane loading past page 1.

Testing Instructions

Load the story editor and scroll the media pane to load a second page. Once it loads, keep scrolling to load page 3.

Addresses #2332

Swap useIntersectionObserver to receive a root node, not a ref.

codecov · 2020-06-09T02:43:32Z

Codecov Report

Merging #2314 into master will increase coverage by 0.47%.
The diff coverage is 80.00%.

@@             Coverage Diff              @@
##             master    #2314      +/-   ##
============================================
+ Coverage     66.13%   66.61%   +0.47%     
- Complexity      314      373      +59     
============================================
  Files           651      664      +13     
  Lines         10768    11267     +499     
============================================
+ Hits           7121     7505     +384     
- Misses         3647     3762     +115

Impacted Files	Coverage Δ	Complexity Δ
...ory/components/library/panes/media/mediaElement.js	`49.25% <ø> (ø)`	`0.00 <0.00> (ø)`
...-story/components/library/panes/media/mediaPane.js	`78.37% <80.00%> (+0.29%)`	`0.00 <0.00> (ø)`
...components/library/panes/animation/animationTab.js	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (ø%)`
...src/edit-story/components/richText/customImport.js	`88.88% <0.00%> (-11.12%)`	`0.00% <0.00%> (ø%)`
.../edit-story/components/panels/stylePreset/utils.js	`93.75% <0.00%> (-6.25%)`	`0.00% <0.00%> (ø%)`
...-story/components/canvas/canvasUploadDropTarget.js	`28.57% <0.00%> (-4.77%)`	`0.00% <0.00%> (ø%)`
assets/src/dashboard/components/button/index.js	`95.83% <0.00%> (-4.17%)`	`0.00% <0.00%> (ø%)`
...c/edit-story/components/inspector/inspectorTabs.js	`17.64% <0.00%> (-3.79%)`	`0.00% <0.00%> (ø%)`
...-story/components/canvas/singleSelectionMovable.js	`47.88% <0.00%> (-2.99%)`	`0.00% <0.00%> (ø%)`
...ews/savedTemplates/savedTemplatesGridView/index.js	`83.33% <0.00%> (-2.39%)`	`0.00% <0.00%> (ø%)`
... and 99 more

github-actions · 2020-06-09T02:46:10Z

Size Change: +6.9 kB (0%)

Total Size: 837 kB

Filename	Size	Change
`assets/js/edit-story.js`	376 kB	+4.23 kB (1%)
`assets/js/stories-dashboard.js`	444 kB	+2.68 kB (0%)

ℹ️ View Unchanged

Filename	Size	Change
`assets/css/edit-story.css`	269 B	0 B
`assets/css/stories-dashboard.css`	305 B	0 B
`assets/css/web-stories-embed-block.css`	501 B	0 B
`assets/js/web-stories-embed-block.js`	15.7 kB	0 B

_{compressed-size-action}

merapi · 2020-06-09T10:06:58Z

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

@@ -30,11 +30,7 @@ function useIntersectionEffect(ref, options = {}, handler, deps = undefined) {
  useEffect(
    () => {
      const node = ref.current;


CMIIW but options.root will always be null here, so while fetching next page works (root=window), correct root is not taken into account. Generally I think we should use ref here, we already talked about this.

PTAL, I took a suggestion from facebook/react#13029. It's not ideal because using a ref callback doesn't provide an easy way of passing that ref around, so I need to introduce another normal ref and set its "current" in the ref callback. Not super happy with this approach so happy to chat over slack about other suggestions you may have!

Looks good.
Extra useRef is not an issue for me, before that you had container, so it's the same in terms of LOC, of course we can have some kind of helper (like people do in #13029) - but not required since we don't have other places that would use it?

swissspidy · 2020-06-09T10:08:57Z

An issue for this bug would be good so it can be appropriately sized, tracked, and QA'ed.

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

merapi

Ideally this should include Karma tests, because this part has been broken several times, making it possible is not trivial (changes in configuration so we can have variable number of elements returned from the API?) but I think we should make it happen.

diegovar · 2020-06-10T04:33:22Z

An issue for this bug would be good so it can be appropriately sized, tracked, and QA'ed.

Done.

diegovar · 2020-06-10T08:30:02Z

Ideally this should include Karma tests, because this part has been broken several times, making it possible is not trivial (changes in configuration so we can have variable number of elements returned from the API?) but I think we should make it happen.

During our standup today we identified this need as well, and we'll be adding karma tests for the media pane in the following weeks.

googlebot · 2020-06-10T10:56:57Z

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.

merapi · 2020-06-10T10:57:25Z

@googlebot I consent

googlebot · 2020-06-10T10:57:31Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

assets/src/edit-story/karma/fixture/fixture.js

merapi · 2020-06-10T11:08:58Z

After chatting with Diego, I pushed some basic test for it, PTAL @diegovar . Optionally we can make it configurable so we can test API 500/empty state.

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

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

diegovar · 2020-06-10T11:49:20Z

After chatting with Diego, I pushed some basic test for it, PTAL @diegovar . Optionally we can make it configurable so we can test API 500/empty state.

This is fantastic, thank you Marcin! I've left a couple of comments but looks great.

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

swissspidy · 2020-06-15T13:00:03Z

@merapi could you help fix the tests here?

googlebot · 2020-06-16T05:01:39Z

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.

barklund · 2020-06-16T05:18:52Z

@googlebot I consent.

googlebot · 2020-06-16T05:19:05Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

merapi

After chatting with Dima tests are now fixed (throw in waitFor), all good.

swissspidy · 2020-06-16T12:02:33Z

This needs testing instructions in order to move forward. They're usually part of the PR template, but here it seems to have been omitted.

merapi · 2020-06-16T16:35:54Z

This needs testing instructions in order to move forward. They're usually part of the PR template, but here it seems to have been omitted.

Yep, leaving that to @diegovar.

diegovar · 2020-06-17T00:20:49Z

This needs testing instructions in order to move forward. They're usually part of the PR template, but here it seems to have been omitted.

Yep, leaving that to @diegovar.

I've added testing instructions.

useIntersectionObserver uses node, not ref

2df1df6

Swap useIntersectionObserver to receive a root node, not a ref.

diegovar requested a review from merapi June 9, 2020 02:42

googlebot added the cla: yes label Jun 9, 2020

merapi reviewed Jun 9, 2020

View reviewed changes

swissspidy added Group: Media Type: Bug Something isn't working labels Jun 9, 2020

merapi reviewed Jun 9, 2020

View reviewed changes

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

merapi suggested changes Jun 9, 2020

View reviewed changes

swissspidy changed the title ~~Fix for loading past page 1 not working introduced in #2069~~ Media: Fix for loading past page 1 Jun 9, 2020

use a ref instead of relying on the node directly

e5b2493

diegovar requested a review from merapi June 10, 2020 04:38

Karma test for fetching 2nd page

94d5a99

googlebot added cla: no and removed cla: yes labels Jun 10, 2020

googlebot added cla: yes and removed cla: no labels Jun 10, 2020

merapi reviewed Jun 10, 2020

View reviewed changes

assets/src/edit-story/karma/fixture/fixture.js Show resolved Hide resolved

diegovar commented Jun 10, 2020

View reviewed changes

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

Add comment explaining rootMargin

d6e8d4d

diegovar commented Jun 10, 2020

View reviewed changes

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

diegovar requested a review from merapi June 10, 2020 11:49

merapi added 2 commits June 10, 2020 14:00

Fix var name

6bb1a46

No focused tests rule

997aeca

diegovar requested review from spacedmonkey and swissspidy as code owners June 10, 2020 12:01

dvoytenko approved these changes Jun 10, 2020

View reviewed changes

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

diegovar removed request for spacedmonkey and swissspidy June 10, 2020 22:59

merapi added 2 commits June 11, 2020 19:58

Will be added in other PR

305d44c

Switch to waitFor

bc0d1cd

swissspidy reviewed Jun 15, 2020

View reviewed changes

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

Added partial karma test

e881153

googlebot added cla: no and removed cla: yes labels Jun 16, 2020

Removing this again - wrong PR

7197019

googlebot added cla: yes and removed cla: no labels Jun 16, 2020

merapi added 2 commits June 16, 2020 10:33

Fix tests (throw error in waitFor until the condition is met)

77c889d

Eslint fix?

709d597

merapi approved these changes Jun 16, 2020

View reviewed changes

swissspidy merged commit 56fed5a into master Jun 17, 2020

swissspidy deleted the fix-loading-not-working branch June 17, 2020 10:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Media: Fix for loading past page 1 #2314

Media: Fix for loading past page 1 #2314

diegovar commented Jun 9, 2020 •

edited

Loading

codecov bot commented Jun 9, 2020 •

edited

Loading

github-actions bot commented Jun 9, 2020 •

edited

Loading

merapi Jun 9, 2020 •

edited

Loading

diegovar Jun 10, 2020

merapi Jun 10, 2020

swissspidy commented Jun 9, 2020

merapi left a comment

diegovar commented Jun 10, 2020

diegovar commented Jun 10, 2020

googlebot commented Jun 10, 2020

merapi commented Jun 10, 2020

googlebot commented Jun 10, 2020

merapi commented Jun 10, 2020

diegovar commented Jun 10, 2020

swissspidy commented Jun 15, 2020

googlebot commented Jun 16, 2020

barklund commented Jun 16, 2020

googlebot commented Jun 16, 2020

merapi left a comment

swissspidy commented Jun 16, 2020

merapi commented Jun 16, 2020

diegovar commented Jun 17, 2020

Media: Fix for loading past page 1 #2314

Media: Fix for loading past page 1 #2314

Conversation

diegovar commented Jun 9, 2020 • edited Loading

Summary

Relevant Technical Choices

To-do

User-facing changes

Testing Instructions

codecov bot commented Jun 9, 2020 • edited Loading

Codecov Report

github-actions bot commented Jun 9, 2020 • edited Loading

merapi Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

diegovar Jun 10, 2020

Choose a reason for hiding this comment

merapi Jun 10, 2020

Choose a reason for hiding this comment

swissspidy commented Jun 9, 2020

merapi left a comment

Choose a reason for hiding this comment

diegovar commented Jun 10, 2020

diegovar commented Jun 10, 2020

googlebot commented Jun 10, 2020

merapi commented Jun 10, 2020

googlebot commented Jun 10, 2020

merapi commented Jun 10, 2020

diegovar commented Jun 10, 2020

swissspidy commented Jun 15, 2020

googlebot commented Jun 16, 2020

barklund commented Jun 16, 2020

googlebot commented Jun 16, 2020

merapi left a comment

Choose a reason for hiding this comment

swissspidy commented Jun 16, 2020

merapi commented Jun 16, 2020

diegovar commented Jun 17, 2020

diegovar commented Jun 9, 2020 •

edited

Loading

codecov bot commented Jun 9, 2020 •

edited

Loading

github-actions bot commented Jun 9, 2020 •

edited

Loading

merapi Jun 9, 2020 •

edited

Loading