Convert interval seconds to days, hours, minutes, and seconds #5220
Conversation
| }) | ||
| .join(""); | ||
| formattedString = formattedString.trim(); | ||
| parts.push(formattedString); |
There was a problem hiding this comment.
The controllflow in this function is quite non-obvious. Please refactor this to not abuse scoping as heavily ^^
There was a problem hiding this comment.
I have refactored the control flow, made it easy and intuitive to understand.
src/util-frontend.js
Outdated
| const toFormattedPart = (value, unitOfTime) => { | ||
| const res = this.getInstance().formatToParts(value, unitOfTime); | ||
| console.log(res); | ||
| let formattedString = res |
There was a problem hiding this comment.
What is happening here? Is this and following lines necessary?
There was a problem hiding this comment.
Refactored, and added comments for better understanding.
There was a problem hiding this comment.
I asked because the filtering you are doing seems ineffective (literal and number are the only alowed values for type).
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/RelativeTimeFormat/formatToParts
Also, I don't think that the trimming is nessesary.
Also the index > 0 is likely incorrect in at least some languages.
I think you are doing this to get rid of the in prefix.
Given that the proper way (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DurationFormat/format) is not yet supported in firefox (https://bugzilla.mozilla.org/show_bug.cgi?id=1648139)
=> I think the best change to do here (after looking into this a bit) would be to switch to the new api and wait for merging until firefox supports it.
| * @param {number} seconds Receive value in seconds. | ||
| * @returns {string} String converted to Days Mins Seconds Format | ||
| */ | ||
| secondsToHumanReadableFormat(seconds) { |
There was a problem hiding this comment.
Could you add a few quick unit tests to ensure that this works as expected?
There was a problem hiding this comment.
I'm seeing e2e tests but no unit tests for frontend, do we have the setup for it?
There was a problem hiding this comment.
Seems like the (previously existing) frontend tests such as the one for currentLocale() (#4692) has been removed without re-adding them.
You can add a folder frontend-test and add a testcase there.
This might need to be added to the npm run .. commands to be executed in CI.
src/util-frontend.js
Outdated
| updateLocale(locale, options = {}) { | ||
| this.locale = locale; | ||
| this.options = { | ||
| ...this.options, | ||
| ...options, | ||
| }; |
There was a problem hiding this comment.
I don't see why we would want to change the options on this. It is shared => would apply globally, right?
| updateLocale(locale, options = {}) { | |
| this.locale = locale; | |
| this.options = { | |
| ...this.options, | |
| ...options, | |
| }; | |
| updateLocale(locale) { | |
| this.locale = locale; |
There was a problem hiding this comment.
Yes it is shared and would be applied globally. Have removed it. Thanks!
src/util-frontend.js
Outdated
| * Default locale and options for Relative Time Formatter | ||
| */ | ||
| constructor() { | ||
| this.locale = currentLocale(); |
There was a problem hiding this comment.
As used, I don't think we need to store the locale.
It is always directly used to construct the RelativeTimeFormat below
=> Let's make this unit a local variable instead
There was a problem hiding this comment.
Right, refactored to get locale while initializing instance.
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma
Tick the checkbox if you understand [x]:
Description
Converts the interval from seconds to "days, hours, minutes and seconds" (depending if it is larger than each) and display the result under the form input. Have ensured it works for both locale directions as well.
Fixes #3601
Type of change
Please delete any options that are not relevant.
Checklist
Screenshots (if any)
Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.