Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PDF completion issues #9776

Merged
merged 3 commits into from
Oct 18, 2022

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Oct 14, 2022

Summary

  • Removes debounce of all handling of the RecycleList update event, and instead just debounces the PDF page rendering which is the expensive call.
  • This allows the currently scrolled to position to be more frequently updated, preventing the error that would occur where visiting entire pages would fail to get recorded.
  • Generalizes the previous fix for 2 page PDFs to the case where the scrollTop is still on the second to last page, but the browser cannot scroll any further. Does this by checking to see if we are on the second to last page, but the browser viewport cannot scroll any further. In this case, we mark both the second to last and last pages as viewed.
  • Removes the PDF control panel autohiding, which is now superfluous following Push PDF pages rendering below full screen bar #9439
  • Fixes an issue that @AllanOXDi and I occasionally saw in testing, whereby the progress_delta being sent to the backend could inadvertently be summed to a value greater than 1.
  • Adds a regression test for the above.

References

Fixes #8868

Reviewer guidance

Continually scroll without stopping from the beginning of a PDF until the end. See that it properly marks as complete.

Zoom out slightly so that more than one page of a PDF is rendered at once. Continually scroll without stopping until the end of the PDF. See that as long as no more than 2 pages are visible on the screen at once, it properly marks as complete.

(This is an edge case where if someone were reading through the PDF in a way that allowed for three or more pages to be visible at once, completion would not be marked - someone would probably need a monitor with an aspect ratio with the height about three times the width at least for this to happen, so I think it's not something to be concerned about).


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added the TODO: needs review Waiting for review label Oct 14, 2022
@rtibbles rtibbles added this to the Planned Patch 7 milestone Oct 14, 2022
@pcenov
Copy link
Member

pcenov commented Oct 17, 2022

@rtibbles I have started testing this one and so far the only issue I've found is that the completion modal gets displayed immediately on opening a 1 page pdf file, thus completely preventing me to even see what the pdf is about.
That seems to be a slight regression as previously it would allow me to at least see the title of the pdf and the completion modal would show only after scrolling down:

2022-10-17_18-10-02.mp4

@rtibbles
Copy link
Member Author

I think properly addressing this would require solving this issue: #9348. I can probably fudge it for now, but just implementing a solution for the above might be easier.

@pcenov
Copy link
Member

pcenov commented Oct 18, 2022

@rtibbles no additional regressions observed during today's round of testing in the rest of the supported browsers and the Android app.

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manual QA passes, experience will be further improved by working on #9348 afterwards 💯 :shipit:

@rtibbles
Copy link
Member Author

OK, have put the above issue into the current patch so that we can try to get this resolved within the same patch. Merging!

@rtibbles rtibbles merged commit fcea00e into learningequality:release-v0.15.x Oct 18, 2022
@rtibbles rtibbles deleted the pretty_darn_fixed branch October 18, 2022 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants