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

feat: add remainingTimeDisplay method #4620

Merged
merged 1 commit into from
Oct 2, 2017

Conversation

brandonocasey
Copy link
Contributor

Description

Allow remainingTime to be given a rounding function during use. Add a function which should be used as the default when displaying remaining time.

Fixes #4608

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 (Chome, Firefox, IE)
  • Reviewed by Two Core Contributors

@brandonocasey brandonocasey force-pushed the feat/better-remaining-time branch from fd889ba to 4e4f779 Compare September 15, 2017 16:47
@gkatsev
Copy link
Member

gkatsev commented Sep 15, 2017

We'd want to update the remainingTimeDisplay to pass in Math.floor (or something else) as well.

@gkatsev
Copy link
Member

gkatsev commented Sep 15, 2017

Also, I'm still not sure this is the direction we want as it requires the user to know how format time works to know what to pass in if the user wants to use it for displaying themselves.

@brandonocasey
Copy link
Contributor Author

brandonocasey commented Sep 15, 2017

After some more exhaustive testing http://jsbin.com/nohawin/1/edit?js,output it seems that floor/floor is the only possible way for this to work. So I think that we shouldn't even provide a custom rounding override option.

@brandonocasey brandonocasey force-pushed the feat/better-remaining-time branch from 4e4f779 to 602768d Compare September 15, 2017 19:58
@gkatsev gkatsev changed the title feat: add remainingTime rounding feat: add remainingTimeDisplay method Sep 19, 2017
@gkatsev gkatsev added the minor This PR can be added to a minor release. It should not be added to a patch release. label Sep 19, 2017
@brandonocasey brandonocasey force-pushed the feat/better-remaining-time branch 2 times, most recently from 85e0f38 to d51dfbb Compare October 2, 2017 20:15
@brandonocasey brandonocasey force-pushed the feat/better-remaining-time branch from d51dfbb to cc952fa Compare October 2, 2017 20:19
@misteroneill misteroneill merged commit 445eb26 into master Oct 2, 2017
@gkatsev gkatsev deleted the feat/better-remaining-time branch March 19, 2018 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor This PR can be added to a minor release. It should not be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rounding errors with player.remainingTime()
3 participants