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

[CalendarPicker] Should limit to target year when looking for the closest enabled date on year change #4834

Closed
2 tasks done
flaviendelangle opened this issue May 11, 2022 · 6 comments
Labels
bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module!

Comments

@flaviendelangle
Copy link
Member

flaviendelangle commented May 11, 2022

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Current behavior 😯

When switching to a new year, the component checks if the new date is disabled, and if so tries to find the closest non-disabled date.

But it does it just like when the prop date is disabled, by checking the day before and after, then 2 days before and after, etc...
So it can quickly select a date outside of the current year.

In #4814 I implemented the correct behavior for the months.
For the year, I need dmtrKovalenko/date-io#614

Expected behavior 🤔

Should go to the closest date but remaining in the year requested.

Steps to reproduce 🕹

=> It selects the 31st of December 2023 instead of the 1st of April 2024

@flaviendelangle flaviendelangle added status: waiting for maintainer These issues haven't been looked at yet by a maintainer bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels May 11, 2022
@flaviendelangle
Copy link
Member Author

@alexfauquette for both the year and the month switch, what do you think should be the behavior when all days in month / year are disabled but the month / year was not disabled.

Right now we fallback to now which feels wrong.
Would you be OK with an error (console error or throw ?) saying No day of the year 2024 is a valid candidate, you should disable it using 'shouldDisableYear' ?

@alexfauquette
Copy link
Member

I'm not sure about the console error because disabled days can come from an external API and so developers will not see them.

About disabling the parent when all the contained dates are disabled, why not. Another option would be to display the requested month containing all the days disabled

image

@flaviendelangle
Copy link
Member Author

About disabling the parent when all the contained dates are disabled, why not

I am not proposing that, because it would mean calling shouldDisableDate for every date in the month for MonthPicker (~30) and every date in the year for YearPicker (~365).

Another option would be to display the requested month containing all the days disabled

OK but which day are you selecting when the user clicks on the month ?

@alexfauquette
Copy link
Member

I am not proposing that, because it would mean calling shouldDisableDate for every date in the month for MonthPicker (~30) and every date in the year for YearPicker (~365).

I thought that's already what is done when looking for the closest day

OK but which day are you selecting when the user clicks on the month?

I do not have this part of the codebase in mind, so it might be not feasible, but do we need to select a date? I would consider that if no date is available it is like when clicking on next/prev month, It only modifies the view, but does not change the value

@flaviendelangle
Copy link
Member Author

I think we are not talking about the same thing.
Checking when the user selects a year and checking before he evens select it to disable the button.

When you are on the YearPicker, we don't check that there is a non-disabled day inside the year.

If the user does <DatePicker shouldDisableDate={date => date.getFullYear() === 2020} />
The year 2020 will not be disabled,

We could call shouldDisableDate for every date of every year currently rendered to disable the year on the year list.
But performance-wise it would be terrible.

I think we have to rely on the fact the the users provided a shouldDisableYear to aggregate the availability himself.


Right now we have a dirty escape gate because when looking for the closest non-disable date, we allow to leave to selected year.

Which can create odd situation.
Imagine the previous selected date was the 2nd of January 2021 and we disable from the 1st to the 10th of January 2020.
The the picker will go to 31st of December 2019 instead of 11th of January 2020.

With this current behavior, when disabling all the year day by day, we will end up in a day of another year.
If we force to get a date of the selected year, then we have a problem.

We could see as you say if it's possible to not selected a date at all (probably keep the old one).
That will not be ideal and the dev should still use shouldDisableYear for an optimal user experience (we could improve the doc). But at least it would be coherent.


Everything explained above also applies to month selection.

@flaviendelangle
Copy link
Member Author

Fixed by #4814

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module!
Projects
None yet
Development

No branches or pull requests

2 participants