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

The progressFrequency is actually a period #317

Closed
ThoAppelsin opened this issue Feb 1, 2018 · 3 comments
Closed

The progressFrequency is actually a period #317

ThoAppelsin opened this issue Feb 1, 2018 · 3 comments

Comments

@ThoAppelsin
Copy link

The progressFrequency prop is actually a period. Wasn't a problem for me, since I have first seen it on the readme#props, where the description says that it is in milliseconds. A default of 1kHz refresh rate would also be too high for a video player, which ascertained me that it really was just a misnomer.

You may consider renaming it to progressPeriod, or make it function as a frequency instead. Or just hope that nobody is mislead by it, at least not for too long.

@cookpete
Copy link
Owner

cookpete commented Feb 2, 2018

I think if it were to change it should be progressInterval, as it quite closely matches the behaviour of the well-known setInterval. However, I also think the chances are very small of someone seeing progressFrequency in the readme but not actually checking what the prop does.

@ThoAppelsin
Copy link
Author

I also think the chances are very small of someone seeing progressFrequency in the readme but not actually checking what the prop does

This is true, but I was thinking that maybe someone could find out what props the component has without the readme. For example, my editor (VSCode) somehow knows about them, such that when I type "p" for an additional prop in your demo application, it recommends me the playsInline and the progressInterval:

vscodeknows

Then again, I also am not sure if it would be beneficial to make any of the two changes I have suggested. It would be good for the newcomers, sure, but would it worth it to make a breaking update for it?

If you are going to take any action, I think renaming the prop would be better, so that it actually breaks for whoever updates, and so they know.

@cookpete
Copy link
Owner

cookpete commented Feb 2, 2018

Then again, I also am not sure if it would be beneficial to make any of the two changes I have suggested. It would be good for the newcomers, sure, but would it worth it to make a breaking update for it?

If I do change it I'll keep progressFrequency working and log a deprecation warning to the console, then remove it whenever 2.0 happens.

david-hub024 pushed a commit to david-hub024/React_VideoPlayer that referenced this issue Dec 23, 2018
progressFrequency still works, but will log a warning to the console
Closes cookpete/react-player#317
david-hub024 pushed a commit to david-hub024/React_VideoPlayer that referenced this issue May 23, 2020
progressFrequency still works, but will log a warning to the console
Closes cookpete/react-player#317
albanqoku added a commit to albanqoku/react-player that referenced this issue Feb 24, 2021
progressFrequency still works, but will log a warning to the console
Closes cookpete/react-player#317
Webmaster1116 added a commit to Webmaster1116/video-player that referenced this issue May 20, 2021
progressFrequency still works, but will log a warning to the console
Closes cookpete/react-player#317
webmiraclepro added a commit to webmiraclepro/video-player that referenced this issue Sep 9, 2022
progressFrequency still works, but will log a warning to the console
Closes cookpete/react-player#317
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants