-
Notifications
You must be signed in to change notification settings - Fork 5
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: #291 Modify method used to calc estimated stt duration #293
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it's taken me so long to review this! Looks good, though I haven't run it. Are you confident the time conversion works for all the various edge cases?
@@ -7,23 +7,21 @@ const updateDescOrder = (firstTr, secondTr) => { | |||
|
|||
const secondsToDhms = (sec) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider changing the name of the function to reflect that it no longer returns days?
const h = Math.floor((seconds % (3600 * 24)) / 3600); | ||
const m = Math.floor((seconds % 3600) / 60); | ||
const s = Math.floor(seconds % 60); | ||
|
||
return [ d, h, m, s ]; | ||
return [ h, m, s ]; | ||
}; | ||
|
||
const ToHumanReadable = (sec) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're editing this file consider starting function name w/lower case t?
|
||
return [ dDisplay, hDisplay, mDisplay, sDisplay ].filter(display => display).join(', '); | ||
return [ hDisplay, mDisplay, sDisplay ].filter(display => display).join(', '); | ||
}; | ||
|
||
const ToDhmsCompact = (sec) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also start w/lower case?
Is your Pull Request request related to another issue in this repository ?
Fixes #291
Describe what the PR does
State whether the PR is ready for review or whether it needs extra work
Ready
Additional context