fix: timepicker on 23 and 25-hour days #4244
Conversation
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was sent to the PullRequest network.
@lemming you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
Overall the changes work, I've made a suggestion about how you can simplify your isSameMinute function to make it more readable. No functional issues are seen with these changes.
Note: While testing things with timezones is hard, there are some good articles that talk about how to build jest mocking to help
Reviewed with ❤️ by PullRequest
src/date_utils.js
Outdated
|
|
||
| return ( | ||
| _date1.getTime() - seconds1 * 1000 - milliseconds1 === | ||
| _date2.getTime() - seconds2 * 1000 - milliseconds2 |
There was a problem hiding this comment.
Ideally you would have a function that takes a date and returns the time without seconds and milliseconds, which you could re-use on two different dates. That said, the overall math of this feels complicated since based on my understanding you should be able to do the following function, which is simpler and involves less code.
function truncToMinute(d: Date) {
return Math.trunc(d.getTime() / 60_000);
}
🔹 Simplify Code (Nice to have)
There was a problem hiding this comment.
Sadly, it won't work for strange historical time zones with subminute offsets.
Example
Prerequisite
Set timezone to 'Europe/Paris'
const toDate = d => new Date(d)
const d1 = new Date(1900, 0, 1, 0, 0);
console.log(d1.toISOString());
//=> '1899-12-31T23:50:39.000Z' ¯\_(ツ)_/¯
const d2 new Date(1900, 0, 1, 0, 0, 40);
console.log(d2);
//=> '1899-12-31T23:51:19.000Z'
function truncToMinute(d) {
return Math.trunc(d.getTime() / 60_000);
}
function isSameMinute(d1, d2) {
const _date1 = toDate(d1);
const _date2 = toDate(d2);
const seconds1 = _date1.getSeconds();
const seconds2 = _date2.getSeconds();
const milliseconds1 = _date1.getMilliseconds();
const milliseconds2 = _date2.getMilliseconds();
return (
_date1.getTime() - seconds1 * 1000 - milliseconds1 ===
_date2.getTime() - seconds2 * 1000 - milliseconds2
);
}
console.log(truncToMinute(d1) === truncToMinute(d2));
//=> false <- WRONG
console.log(isSameMinute(d1, d2));
//=> true <- RIGHTBut I can introduce startOfMinute function to make code more concise. date-fns's version can't be used here as it suffers from the same bug with DST.
There was a problem hiding this comment.
Might be worth adding in a comment in code to note down the edge case permanently, I can see this being accidentally refactored into a regression in the future, consider the relatively niche timezone-related knowledge.
There was a problem hiding this comment.
Great suggestion! Added comments and a few tests (not yet related to time zones and daylight-saving time quirks, unfortunately)
There was a problem hiding this comment.
I agree with the feedback left by David, and would recommend applying those changes.
Reviewed with ❤️ by PullRequest
There was a problem hiding this comment.
Solid improvement, a few more suggestions. My goal as a reviewer is to help create code that is easy for future readers to understand quickly.
Reviewed with ❤️ by PullRequest
|
@lemming can you take a look at the test failures? |
There was a problem hiding this comment.
PullRequest reviewed the updates made to #4244 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.
Reviewed by:
Sure |
There was a problem hiding this comment.
PullRequest reviewed the updates made to #4244 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.
Reviewed by:
Codecov Report
@@ Coverage Diff @@
## main #4244 +/- ##
=======================================
Coverage 96.33% 96.34%
=======================================
Files 25 25
Lines 2346 2351 +5
Branches 959 957 -2
=======================================
+ Hits 2260 2265 +5
Misses 86 86
|
There was a problem hiding this comment.
PullRequest reviewed the updates made to #4244 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.
Reviewed by:
This is a fix for #4243.
Notes
Tests
I would gladly add tests but time zones are involved which makes things complicated. In Node.js timezone could be changed only with environment variable which requires significant changes in test running process if we want to test several of them.
timeClassName prop
Signature of
timeClassNamechanged as currH and currM don't make sense anymore. This is a potentially breaking change and also add inconvenience for the user willing to highlight selected time.Time range
Time range is also prone to DST bugs (if someone accidentally would set a date on the DST boundary) and the interface looks somewhat strange to me. I think it should either accept an array of ranges or start and end times in
HH:mmformat.