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 set current time to keep the same behavior for all players #5416

Merged
merged 4 commits into from Aug 11, 2022

Conversation

renatho
Copy link
Contributor

@renatho renatho commented Aug 4, 2022

Based on #5410

Changes proposed in this Pull Request

  • It updates the setCurrentTime, in order to work in the same way for all the players. Basically, we need to have played a video in some players in order to have the frame updated when calling the setCurrentTime, so if the video wasn't played yet, we just play and pause to make it work.

Testing instructions

  • Check this branch 1619-gh-Automattic/sensei-pro
  • Before playing the videos, add points in the same place, and make sure the video frame will update.
  • For YT, it's not working for me in the first frame (it keeps loading), but I don't think there's something we can do about that.

@renatho renatho self-assigned this Aug 4, 2022
@renatho renatho added this to the 4.6.1 milestone Aug 4, 2022
@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

Merging #5416 (e249111) into trunk (b2a3fe2) will increase coverage by 0.00%.
The diff coverage is 1.61%.

Impacted file tree graph

@@            Coverage Diff            @@
##              trunk    #5416   +/-   ##
=========================================
  Coverage     44.55%   44.56%           
  Complexity     8728     8728           
=========================================
  Files           412      412           
  Lines         31115    31115           
  Branches        234      234           
=========================================
+ Hits          13864    13865    +1     
+ Misses        17078    17077    -1     
  Partials        173      173           
Impacted Files Coverage Δ
includes/admin/class-sensei-learners-main.php 4.96% <0.00%> (ø)
...cludes/class-sensei-analysis-lesson-list-table.php 0.00% <0.00%> (ø)
.../class-sensei-analysis-user-profile-list-table.php 0.00% <0.00%> (ø)
includes/class-sensei-analysis.php 8.90% <0.00%> (ø)
includes/class-sensei-grading-main.php 0.00% <0.00%> (ø)
includes/class-sensei-grading.php 11.72% <0.00%> (ø)
includes/class-sensei-list-table.php 9.09% <0.00%> (ø)
...ss-sensei-reports-overview-list-table-abstract.php 64.66% <4.54%> (ø)
.../class-sensei-learners-admin-bulk-actions-view.php 54.71% <5.26%> (ø)
...cludes/class-sensei-analysis-course-list-table.php 26.61% <7.14%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f65a7bc...e249111. Read the comment docs.

@renatho renatho requested a review from a team August 4, 2022 20:36
@renatho renatho marked this pull request as ready for review August 4, 2022 20:36
@renatho renatho changed the title Fix set current time for vimeo Fix set current time to keep the same behavior for all players Aug 4, 2022
/**
* Variable that saves the last current time.
*/
let lastCurrentTime;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just moved, renamed, and documented.

Base automatically changed from add/current-time-to-player-api to trunk August 5, 2022 16:06
@renatho renatho modified the milestones: 4.6.1, 4.6.2 Aug 10, 2022
Copy link
Contributor

@aaronfc aaronfc left a comment

Choose a reason for hiding this comment

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

Looks good and works well! 🚀

@renatho renatho merged commit 5a54a38 into trunk Aug 11, 2022
@renatho renatho deleted the fix/set-current-time branch August 11, 2022 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants