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.

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: Ensure date doesn't updates when selecting month and year from dropdown #10163

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

muraliSingh7
Copy link
Contributor

As demonstrated by @Devessier in twentyhq/core-team-issues#383.
I have made some fixes in DatePicker. Now, the date will remain unchanged when a new month or year is selected.

screen-capture.webm

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR fixes a bug in the DateTimePicker component where selecting a new month or year from the dropdown would unintentionally modify the selected date.

  • Added state management (datePickerMonth, datePickerYear) in InternalDatePicker.tsx to track calendar view independently from selected date
  • Modified openToDate calculation in InternalDatePicker.tsx to use local state instead of selected date
  • Simplified AbsoluteDatePickerHeader.tsx by removing DateTime calculations and using direct month/year props
  • Added navigation handlers (onAddMonth, onSubtractMonth) that update view without affecting selection
  • Improved dropdown behavior by separating view state from date selection state

2 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines 419 to 429
const openToDate = DateTime.now()
.set({
day: dateParsed.get('day'),
month: datePickerMonth + 1,
year: datePickerYear,
hour: 23,
minute: 59,
second: 59,
millisecond: 999,
})
.toJSDate();
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: DateTime.now() could cause issues if the current time is near midnight, as it might show a different date than intended. Consider using the internalDate as base instead of now()

Suggested change
const openToDate = DateTime.now()
.set({
day: dateParsed.get('day'),
month: datePickerMonth + 1,
year: datePickerYear,
hour: 23,
minute: 59,
second: 59,
millisecond: 999,
})
.toJSDate();
const openToDate = DateTime.fromJSDate(internalDate)
.set({
day: dateParsed.get('day'),
month: datePickerMonth + 1,
year: datePickerYear,
hour: 23,
minute: 59,
second: 59,
millisecond: 999,
})
.toJSDate();

Comment on lines +411 to +417
const [datePickerMonth, setDatePickerMonth] = useState(
endOfDayInLocalTimezone.getMonth(),
);

const [datePickerYear, setDatePickerYear] = useState(
endOfDayInLocalTimezone.getFullYear(),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider adding useEffect to sync datePickerMonth/Year when internalDate changes to prevent stale view state

Comment on lines 31 to 32
month?: number;
year?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: month and year props should be required since they are used directly in Select components without fallbacks

Suggested change
month?: number;
year?: number;
month: number;
year: number;

@lucasbordeau lucasbordeau self-requested a review February 12, 2025 15:27
Copy link
Contributor

@lucasbordeau lucasbordeau left a comment

Choose a reason for hiding this comment

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

Could you please fix the CI failing tests ?

setDatePickerYear(year);
};

const onSubtractMonth = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

all handlers should begin with handle :

remember that => onEvent=handleEvent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have fixed the naming issue.

@muraliSingh7
Copy link
Contributor Author

Could you please fix the CI failing tests ?

image

@lucasbordeau
Copy link
Contributor

We need to change the displayed date in the picker if we type in a different date, this is why the tests fail.

@muraliSingh7
Copy link
Contributor Author

Why storybook command as specified in docs is unable to run locally ?

@@ -454,6 +454,14 @@ export const DateTimePicker = ({
}
};

useEffect(() => {
const month = dateParsed.get('month');
Copy link
Contributor

Choose a reason for hiding this comment

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

Please find another pattern without a useEffect, it's a bad practice to synchronize state with useEffect, and in that case we can certainly avoid it.

You can do it by placing setDatePickerMonth and setDatePickerYear in the right handlers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I am planning to replace onChange in DateTimeInput and AbsoluteDatePickerHeader with handleDateChange in InternalDatePicker. I have modified handleDateChange sightly:


  const handleDateChange = (date: Date | null) => {
    if (!date) {
      onChange?.(null);
      return;
    }

    const dateParsed = DateTime.fromJSDate(internalDate, {
      zone: isDateTimeInput ? timeZone : 'local',
    })
      .set({
        day: date.getDate(),
        month: date.getMonth() + 1,
        year: date.getFullYear(),
      })
      .toJSDate();

    handleChangeMonth(dateParsed.getMonth());
    handleChangeYear(dateParsed.getFullYear());
    onChange?.(date);
  };

I have tested it is working but I want to know why we are not using it and why we are using onChange straight away ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please let's not change the handler strategy because there are many moving parts and this setup is working so your change has a high risk of introducing regression. Let's only add calls to setDatePickerMonth and setDatePickerYear where needed.

@lucasbordeau
Copy link
Contributor

Why storybook command as specified in docs is unable to run locally ?

They are but they can be flaky depending on your hardware.

@muraliSingh7
Copy link
Contributor Author

muraliSingh7 commented Feb 24, 2025

@lucasbordeau There is FormDateTimeField and FormDateFieldComponent which has their own input to change datetime. The value from those input is passed to DateTimePicker via props called date. Without useEffect you cannot change month and year state variable for these change to propagate or please suggest some methods via which we can achieve it.

This in turn causing two stories to fail the DefaultsToMaxValueWhenTypingReallyFarDate and DefaultsToMinValueWhenTypingReallyOldDate.

Actually there is one more issue which is causing the above DefaultsToMaxValueWhenTypingReallyFarDate stories to fail. The year dropdown has limited values currently the upper limit of year is upto 2030 in AbsoluteDatePicker Component.

const years = Array.from(
  { length: 200 },
  (_, i) => new Date().getFullYear() + 5 - i,
).map((year) => ({ label: year.toString(), value: year }));

If you want from 1900 to 2100 we need to change the above code to this :

const years = Array.from(
  { length: 201 },
  (_, i) => new Date().getFullYear() + 75 - i,
).map((year) => ({ label: year.toString(), value: year }));

But still this will fix test case for temporary we need to make MIN_DATE and MAX_DATE to change when current year changes.

There are some regex issue too in some of the stories like Default and Disabled in FormDateTimeFieldInput.stories.
Change new RegExp(12/09/${currentYear} \d{2}:20) to new RegExp(12/09/${currentYear} \d{2}:\d{2})

@Devessier
Copy link
Contributor

@muraliSingh7, I created these components. I will give it a look!

@Devessier
Copy link
Contributor

There is FormDateTimeField and FormDateFieldComponent which has their own input to change datetime. The value from those input is passed to DateTimePicker via props called date. Without useEffect you cannot change month and year state variable for these change to propagate or please suggest some methods via which we can achieve it.

I tested your changes, and it's the expected behavior.

@Devessier
Copy link
Contributor

The calendar does not update when you select a day unavailable in the previous month and then return to the last month.

In my demo, when I select the 31st of October and go to September, if I click on a day, it will choose this day of October, not September. The view of the calendar was not updated when I switched to September.

CleanShot.2025-02-24.at.10.55.31.mp4

… stories involving DateTimeField as well as updated years array logic
@muraliSingh7
Copy link
Contributor Author

@Devessier The date in openToDate logic was wrong thats why the navigation and dates not present in current month was showing up for selection. I have fixed and tested it from my side now it is not showing that issue. Can you please check and confirm whether the issue is fixed or not?

@Devessier
Copy link
Contributor

Devessier commented Feb 24, 2025

Identified issues

Expected the next month button to be disabled when user has reached the max date

CleanShot.2025-02-24.at.18.24.58.mp4

Try to use the functions from react-date-picker to go prev/next month

https://reactdatepicker.com/#example-custom-header

Try to use:

decreaseMonth
increaseMonth
prevMonthButtonDisabled
nextMonthButtonDisabled

These functions should limit the edge cases.

@Devessier
Copy link
Contributor

Thank you a lot for your work, @muraliSingh7. I listed a few things you can check.

I can provide some guidance about the failing stories. Would you be willing to review them? Let me know.

@muraliSingh7
Copy link
Contributor Author

muraliSingh7 commented Feb 24, 2025

@Devessier Yes sure I will review them. Please let me know what can be done to fix the failing stories issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants