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: Fix playback rate iteration if rates are not in the ascending order #7618

Conversation

BruceRodrigues
Copy link
Contributor

@BruceRodrigues BruceRodrigues commented Jan 20, 2022

On the playback rate button's click, the iteration over all possible rates will not happen as expected. E.g. if you follow youtube's pattern ([ 2, 1.75, 1.5, 1.25, 1, 0.75, 0.5, 0.25 ]), and click on the playback rate button it will change to 2x and you won't be able to change it anymore.

Description

Fixes #7617

Specific Changes proposed

The iteration over the rates array are now done by the array's index.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

… order

On the playback rate button's click, the iteration over all possible rates will not happen as expected. E.g. if you follow youtube's pattern ([ 2, 1.75, 1.5, 1.25, 1, 0.75, 0.5, 0.25 ]), and click on the playback rate button it will change to 2x and you won't be able to change it anymore.

Fixes videojs#7617
@welcome
Copy link

welcome bot commented Jan 20, 2022

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@mister-ben
Copy link
Contributor

Thanks. This makes sense, but it would be good to add a test.

@BruceRodrigues
Copy link
Contributor Author

Thanks. This makes sense, but it would be good to add a test.

Sure thing! I'll do that!

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the PR. Would you mind adding some tests to verify? Nevermind, saw above comment after submitting.

@BruceRodrigues
Copy link
Contributor Author

Nice, thanks for the PR. Would you mind adding some tests to verify? Nevermind, saw above comment after submitting.

Hey guys! Sorry for the late reply. Just added some tests to check the playback rate iteration.

@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #7618 (c16a39a) into main (53cbfc6) will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7618      +/-   ##
==========================================
+ Coverage   80.24%   80.36%   +0.11%     
==========================================
  Files         116      116              
  Lines        7325     7322       -3     
  Branches     1771     1770       -1     
==========================================
+ Hits         5878     5884       +6     
+ Misses       1447     1438       -9     
Impacted Files Coverage Δ
...ar/playback-rate-menu/playback-rate-menu-button.js 96.29% <100.00%> (+15.59%) ⬆️

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 53cbfc6...c16a39a. Read the comment docs.

@misteroneill misteroneill changed the title Fix: Fixing playback rate iteration if rates are not in the ascending order fix: Fix playback rate iteration if rates are not in the ascending order Feb 22, 2022
Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

Seems solid and I tested it manually and works as advertised! 👍

@misteroneill misteroneill added needs: LGTM Needs one or more additional approvals patch This PR can be added to a patch release. and removed needs: updates labels Feb 22, 2022
Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Thanks!

@misteroneill misteroneill merged commit 50fe5f6 into videojs:main Feb 23, 2022
@welcome
Copy link

welcome bot commented Feb 23, 2022

Congrats on merging your first pull request! 🎉🎉🎉

@misteroneill misteroneill added confirmed and removed needs: LGTM Needs one or more additional approvals labels Feb 23, 2022
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Playback rate button doesn't iterate over the rates if the array doesn't follow a ascending order
4 participants