-
Notifications
You must be signed in to change notification settings - Fork 203
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: optimize scroll position observer after video fullscreen exit #1371
fix: optimize scroll position observer after video fullscreen exit #1371
Conversation
Thanks for the pull request, @ihor-romaniuk! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
c7cb0bb
to
79f0b6a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1371 +/- ##
=======================================
Coverage 88.28% 88.28%
=======================================
Files 292 292
Lines 5002 5002
Branches 1269 1269
=======================================
Hits 4416 4416
Misses 570 570
Partials 16 16 ☔ View full report in Codecov by Sentry. |
Hey @ihor-romaniuk, thank you for this contribution! I marked it as ready for review, but let me know if you were planning to work on the changes some more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me.
@ihor-romaniuk 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Hey @arbrandes, would you mind giving the Quince backport for this PR (#1370) a quick look, too? |
@arbrandes and also take a look at Redwood backport PR #1405, please. |
This merge request contains an optimization for toggling video xblock full-screen mode and saving the previous window top offset position on exit from the full-screen state.
With this refactor, we call
scroll
after exit full-screen mode only and not depends on xblock resize because sometimes the resize event comes before exiting full screen mode, and we don't need to call scroll every time on resize.Related PR to quince branch: #1370