Show a selector for initial duration at job launch - #721
Conversation
…-existing calculation method
psarando
left a comment
psarando
left a comment
There was a problem hiding this comment.
LGTM 👍
I just had some questions and minor comments.
| const SECONDS_PER_HOUR = 3600; | ||
| const SECONDS_PER_DAY = 86400; |
There was a problem hiding this comment.
I personally prefer the less-ambiguous raw calculations over the results (60 * 60 and 60 * 60 * 24).
I once read an article that gave a metric of how common the constant 84600 was across github 😅
But not necessary to update this here.
| const SECONDS_PER_MINUTE = 60; | ||
| const SECONDS_PER_HOUR = 60 * SECONDS_PER_MINUTE; | ||
| const SECONDS_PER_DAY = 24 * SECONDS_PER_HOUR; |
| /** | ||
| * Formats a duration given in seconds as a human-readable string. | ||
| * | ||
| * @param {number} seconds - The duration in seconds. | ||
| * @returns {string} - The formatted duration (e.g. "12 hours", "3 days"). | ||
| */ | ||
| const formatDuration = (seconds) => { | ||
| if (!seconds) { | ||
| return ""; | ||
| } | ||
| const days = Math.floor(seconds / SECONDS_PER_DAY); | ||
| seconds %= SECONDS_PER_DAY; | ||
| const hours = Math.floor(seconds / SECONDS_PER_HOUR); | ||
| seconds %= SECONDS_PER_HOUR; | ||
| const minutes = Math.floor(seconds / SECONDS_PER_MINUTE); | ||
| return formatDurationStr({ days, hours, minutes }); | ||
| }; |
There was a problem hiding this comment.
Nice 👍
Eventually, if anything else needs to use this function, it could move into components/utils/DateFormatter, but that's not necessary now.
| */ | ||
| function InitialDurationField({ baseId, maxTimeLimitSeconds }) { | ||
| const { t } = useTranslation("launch"); | ||
| const { values } = useFormikContext(); |
There was a problem hiding this comment.
I keep forgetting about useFormikContext 👍
|
Thanks for the review! I'll go ahead and merge this since I think I've mopped up the remaining issues of my own comprehension. If you have more thoughts we can still make changes of course. |
|
LGTM 👍 🎉 |
2 participants
(defaulting to pre-existing calculation method)
depends on cyverse-de/timelord#31 cyverse-de/common-swagger-api#100 cyverse-de/apps#302
This is the one of this batch I'm least sure of, but I think it should be alright. The calculation of the dropdown options is a bit funky though, and hopefully the field shows at the correct times (or not).