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.

Sign up for GitHub

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: timepicker on 23 and 25-hour days #4244

Merged
merged 6 commits into from
Sep 15, 2023
Merged

fix: timepicker on 23 and 25-hour days #4244

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2dda767
fix: timepicker on 23 and 25-hour days
lemming Sep 10, 2023
101e166
remove extra console.log and refactor isSameMinute
lemming Sep 11, 2023
bfb1a6b
refactor isSameMinute
lemming Sep 11, 2023
5d8d4ac
chore: refactor isSameMinute in more idiomatic way
lemming Sep 11, 2023
3bdf133
restore focus logic in timepicker
lemming Sep 12, 2023
0ea56aa
add JSDoc and tests for new date utils
lemming Sep 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/date_utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -818,3 +818,30 @@ export function getYearsPeriod(
const startPeriod = endPeriod - (yearItemNumber - 1);
return { startPeriod, endPeriod };
}

export function getHoursInDay(d) {
const startOfDay = new Date(d.getFullYear(), d.getMonth(), d.getDate());
const startOfTheNextDay = new Date(
d.getFullYear(),
d.getMonth(),
d.getDate(),
24
);

return Math.round((+startOfTheNextDay - +startOfDay) / 3_600_000);
}

export 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Image of David K David K

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 <- RIGHT

But 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion! Added comments and a few tests (not yet related to time zones and daylight-saving time quirks, unfortunately)

);
}
10 changes: 6 additions & 4 deletions src/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,10 +686,12 @@ export default class DatePicker extends React.Component {
const selected = this.props.selected
? this.props.selected
: this.getPreSelection();
let changedDate = setTime(selected, {
hour: getHours(time),
minute: getMinutes(time),
});
let changedDate = this.props.selected
? time
: setTime(selected, {
hour: getHours(time),
minute: getMinutes(time),
});

this.setState({
preSelection: changedDate,
Expand Down
60 changes: 25 additions & 35 deletions src/time.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,15 @@ import PropTypes from "prop-types";
import {
getHours,
getMinutes,
setHours,
setMinutes,
newDate,
getStartOfDay,
addMinutes,
formatDate,
isBefore,
isEqual,
isTimeInDisabledRange,
isTimeDisabled,
timesToInjectAfter,
getHoursInDay,
isSameMinute,
} from "./date_utils";

export default class Time extends React.Component {
Expand Down Expand Up @@ -91,20 +89,23 @@ export default class Time extends React.Component {
this.props.onChange(time);
};

isSelectedTime = (time, currH, currM) =>
this.props.selected &&
currH === getHours(time) &&
currM === getMinutes(time);
isSelectedTime = (time) => {
const result =
this.props.selected && isSameMinute(this.props.selected, time);

liClasses = (time, currH, currM) => {
if (result) {
console.log(this.props.selected, time);
Zarthus marked this conversation as resolved.
Show resolved Hide resolved
Zarthus marked this conversation as resolved.
Show resolved Hide resolved
}
return result;
};

liClasses = (time) => {
let classes = [
"react-datepicker__time-list-item",
this.props.timeClassName
? this.props.timeClassName(time, currH, currM)
: undefined,
this.props.timeClassName ? this.props.timeClassName(time) : undefined,
];

if (this.isSelectedTime(time, currH, currM)) {
if (this.isSelectedTime(time)) {
classes.push("react-datepicker__time-list-item--selected");
}

Expand Down Expand Up @@ -160,19 +161,18 @@ export default class Time extends React.Component {
const format = this.props.format ? this.props.format : "p";
const intervals = this.props.intervals;

const base = getStartOfDay(newDate(this.props.selected));
const multiplier = 1440 / intervals;
const activeDate =
this.props.selected || this.props.openToDate || newDate();

const base = getStartOfDay(activeDate);
const sortedInjectTimes =
this.props.injectTimes &&
this.props.injectTimes.sort(function (a, b) {
return a - b;
});

const activeDate =
this.props.selected || this.props.openToDate || newDate();
const currH = getHours(activeDate);
const currM = getMinutes(activeDate);
const activeTime = setHours(setMinutes(base, currM), currH);
const minutesInDay = 60 * getHoursInDay(activeDate);
const multiplier = minutesInDay / intervals;

for (let i = 0; i < multiplier; i++) {
const currentTime = addMinutes(base, i * intervals);
Expand All @@ -190,34 +190,24 @@ export default class Time extends React.Component {
}
}

// Determine which time to focus and scroll into view when component mounts
const timeToFocus = times.reduce((prev, time) => {
if (isBefore(time, activeTime) || isEqual(time, activeTime)) {
return time;
} else {
return prev;
}
}, times[0]);

return times.map((time, i) => {
const isActiveTime = isSameMinute(time, activeDate);
return (
<li
key={i}
onClick={this.handleClick.bind(this, time)}
className={this.liClasses(time, currH, currM)}
className={this.liClasses(time)}
ref={(li) => {
if (time === timeToFocus) {
if (isActiveTime) {
this.centerLi = li;
}
}}
onKeyDown={(ev) => {
this.handleOnKeyDown(ev, time);
}}
tabIndex={time === timeToFocus ? "0" : "-1"}
tabIndex={isActiveTime ? 0 : -1}
role="option"
aria-selected={
this.isSelectedTime(time, currH, currM) ? "true" : undefined
}
aria-selected={this.isSelectedTime(time) ? "true" : undefined}
>
{formatDate(time, format, this.props.locale)}
</li>
Expand Down
Loading