-
Notifications
You must be signed in to change notification settings - Fork 223
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/scroll position on exit from video fullscreen mode maple #982
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. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## open-release/maple.master #982 +/- ##
=============================================================
+ Coverage 84.01% 84.03% +0.02%
=============================================================
Files 226 226
Lines 3986 3991 +5
Branches 1015 1018 +3
=============================================================
+ Hits 3349 3354 +5
Misses 615 615
Partials 22 22
☔ View full report in Codecov by Sentry. |
@ihor-romaniuk Thank you for your contribution! Please let us know when it is ready for review. |
@itsjeyd It's ready for review. Thanks in advance. |
Hi @mattcarter, could we please get this PR lined up for engineering review by the Aurora team? |
@ProductRyan Since the corresponding edx-platform PR (openedx/edx-platform#31055) has been flagged for product review, I'm tentatively flagging this PR for product review as well. If that's not necessary, please let me know. |
Hey @jmakowski1123, you might already be aware of this PR but I'll CC you here (and on a few other PRs) just in case. |
@ihor-romaniuk While #981 is going through product review, it might make sense to address failing codecov checks on this PR and #983. That way the changes would be ready for engineering review right after they get approval from product. |
@mphilbrick211 I'm going to remove the "product review" label from this issue as the scope for this ticket is on code checks. Product review is underway on #981 |
For reference, product review for this feature is happening via openedx/platform-roadmap#229. |
@ihor-romaniuk Just a quick heads-up, I'm updating labels on this PR again to indicate that it's waiting for additional changes addressing codecov failures. |
Hi @itsjeyd. Sorry for delay. Tests were updated and it's ready for review. |
Thanks @ihor-romaniuk, I posted on the main PR (#983) to get the changes lined up for engineering review. |
Note on current status: Main PR has been added to review queue. |
Why is |
Hey @ihor-romaniuk, a friendly reminder to address @leangseu-edx's comment above so we can get this backport merged :) |
b17bb67
to
8018989
Compare
Hi @leangseu-edx @itsjeyd |
Thanks @ihor-romaniuk, looks like we should be able to get this over the line very soon. |
Hey @leangseu-edx, friendly ping about having a quick look at this PR. It's a backport of another PR (#983) that you recently reviewed and merged. |
This is a pair pr with openedx/edx-platform#31055. Due to the other pr if failing, I am uncomfortable merging it. |
@leangseu-edx OK I see, thanks for the info. |
Closed due to outdated edx version and no need for these changes. |
@ihor-romaniuk Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
This merge request contains a fix for toggling video xblock full-screen mode and saving the previous window top offset position on exit from the full-screen state.
A related bug was found here https://bugs.chromium.org/p/chromium/issues/detail?id=142427 but it still reproduces.
Realised solution: Save the scroll position before the turn on the fullscreen mode and scroll to the previous position on turn off the fullscreen mode.
Dependent PR to MFE Learning:
This MR openedx/edx-platform#31055 must be merged with this MR.