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

Jump to bottom

fix(CalendarMonth): does not skip a month on arrow click #9722

Merged
merged 5 commits into from
Oct 18, 2023
Merged

fix(CalendarMonth): does not skip a month on arrow click #9722

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8159ce5
fix(CalendarMonth): correct setting of initialDate
adamviktora Oct 9, 2023
8107be7
fix(CalendarMonth): keep the dates unfocused on month change
adamviktora Oct 9, 2023
0de2966
fix(CalendarMonth): unfocus date on year change
adamviktora Oct 9, 2023
2802604
fix(CalendarMonth): undefined check
adamviktora Oct 15, 2023
7a25128
refactor(CalendarMonth): remove unnecessary undefined check
adamviktora Oct 18, 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
60 changes: 17 additions & 43 deletions packages/react-core/src/components/CalendarMonth/CalendarMonth.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,12 +154,12 @@
const [isSelectOpen, setIsSelectOpen] = React.useState(false);

const getInitialDate = () => {
const initDate = new Date(dateProp);
if (isValidDate(initDate)) {
return initDate;
} else {
return isValidDate(rangeStart) ? rangeStart : today;
if (dateProp && isValidDate(dateProp)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: is checking that dateProp is truthy needed here? It looks like isValidDate should return false anyways if it's undefined.

Copy link
Contributor Author

@adamviktora adamviktora Oct 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, you are right, the previous issue lied in this line: const initDate = new Date(dateProp); where we create a valid date object with initial value (1st Jan 1970 / 31st Dec 1969), even if dateProp is undefined (for some reason if I console log dateProp, it prints out false and new Date(false) returns a valid date with that initial value).

So now that we check directly the dateProp, it is not needed.

Copy link
Contributor Author

@adamviktora adamviktora Oct 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

anyway, my ESLint is shouting, because the isValidDate function only accepts Date and not Date | undefined, so can I keep it as it is?

Copy link
Contributor

Choose a reason for hiding this comment

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

You could also just change isValidDate to have the argument optional since it checks for a truthy value internally, up to you though.

return dateProp;
} else if (rangeStart && isValidDate(rangeStart)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question for rangeStart here.

return rangeStart;
}
return today;
};
const initialDate = getInitialDate();
const [focusedDate, setFocusedDate] = React.useState(initialDate);
Expand All @@ -176,14 +176,14 @@
} else if (!dateProp) {
setFocusedDate(today);
}
}, [dateProp]);

Check warning on line 179 in packages/react-core/src/components/CalendarMonth/CalendarMonth.tsx

View workflow job for this annotation

GitHub Actions / lint 

React Hook useEffect has a missing dependency: 'focusedDate'. Either include it or remove the dependency array

useEffect(() => {
// Calendar month should not be focused on page load
if ((shouldFocus || isDateFocused) && focusedDateValidated && focusRef.current) {
focusRef.current.focus();
}
}, [focusedDate, isDateFocused, focusedDateValidated, focusRef]);

Check warning on line 186 in packages/react-core/src/components/CalendarMonth/CalendarMonth.tsx

View workflow job for this annotation

GitHub Actions / lint 

React Hook useEffect has a missing dependency: 'shouldFocus'. Either include it or remove the dependency array

const onMonthClick = (ev: React.MouseEvent, newDate: Date) => {
setFocusedDate(newDate);
Expand Down Expand Up @@ -211,43 +211,25 @@
}
};

const changeMonth = (month: number) => {
const newDate = new Date(focusedDate);
const desiredDay = newDate.getDate();
const monthDays = new Date(newDate.getFullYear(), (month + 1) % 12, 0).getDate(); // Setting day 0 of the next month returns the last day of current month

if (monthDays < desiredDay) {
newDate.setDate(monthDays);
}

newDate.setMonth(month);
const changeYear = (newYear: number) => changeMonth(focusedDate.getMonth(), newYear);

if (initialDate.getDate() > desiredDay && monthDays > desiredDay) {
newDate.setDate(initialDate.getDate());
}

return newDate;
};
const changeMonth = (newMonth: number, newYear?: number) =>
new Date(newYear ?? focusedDate.getFullYear(), newMonth, 1);

const addMonth = (toAdd: -1 | 1) => {
let newMonth = new Date(focusedDate).getMonth() + toAdd;
let newMonth = focusedDate.getMonth() + toAdd;
let newYear = focusedDate.getFullYear();

if (newMonth === -1) {
newMonth = 11;
newYear--;
} else if (newMonth === 12) {
newMonth = 0;
newYear++;
}
const newDate = changeMonth(newMonth);
if (toAdd === 1 && newMonth === 0) {
newDate.setFullYear(newDate.getFullYear() + 1);
}
if (toAdd === -1 && newMonth === 11) {
newDate.setFullYear(newDate.getFullYear() - 1);
}
return newDate;
};

const yearHasFebruary29th = (year: number) => new Date(year, 1, 29).getMonth() === 1;
const dateIsFebruary29th = (date: Date) => date.getMonth() === 1 && date.getDate() === 29;
return changeMonth(newMonth, newYear);
};

const prevMonth = addMonth(-1);
const nextMonth = addMonth(1);
Expand Down Expand Up @@ -340,19 +322,11 @@
type="number"
value={yearFormatted}
onChange={(ev: React.FormEvent<HTMLInputElement>, year: string) => {
const newDate = new Date(focusedDate);
if (dateIsFebruary29th(newDate) && !yearHasFebruary29th(+year)) {
newDate.setDate(28);
newDate.setMonth(1);
}
if (dateIsFebruary29th(initialDate) && yearHasFebruary29th(+year)) {
newDate.setFullYear(+year);
newDate.setDate(29);
}
newDate.setFullYear(+year);
const newDate = changeYear(Number(year));
setFocusedDate(newDate);
setHoveredDate(newDate);
setShouldFocus(false);
focusRef.current.blur(); // will unfocus a date when changing year via up/down arrows
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be necessary. Clicking the input arrows for the year will place focus on the input itself

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like google calendar does not focus a date as scrolling through months. Adam had pinged me about htis yesterday and the PF 5.0 version did nit focus a specific month.

onMonthChange(ev, newDate);
}}
/>
Expand Down
Loading