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: disable outside days outside the before and end month range #2578

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rodgobbi
Copy link
Contributor

@rodgobbi rodgobbi commented Nov 10, 2024

Issue this PR fixes

Current behavior

When using both showOutsideDays prop equal to true together with a startMonth date, the days before the startMonth month are not disabled in the calendar and can be clicked and selected.

Screen recording of the issue using the StartEndMonthsShowOutsideDays example in this MR, the month March of 2024 is the only enabled, and the 10th day is disabled for showing how a disabled date behaves in the calendar:

startMonth.and.endMonth.issues.mov

Expected behavior

Days before the startMonth month or after the endMonth month should be disabled in the calendar.

Screen recording of the fix using the StartEndMonthsShowOutsideDays example in this MR:

Disabled.outside.days.mov

What's Changed

Add logic to flag the days before the startMonth month or after the endMonth month as disabled in the src/useGetModifiers.tsx.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Additional Notes

@gpbl This PR starts as a draft because I need to discuss with you what should be the expected behavior and I'll suggest a behavior change based on what I discovered when fixing the issue.
I'll follow-up in the PR comments.

@rodgobbi
Copy link
Contributor Author

@gpbl
While working to fix the issue, I discovered different behaviors for the days outside the startMonth and endMonth months.
More precisely, the days before the startMonth month are displayed in the calendar when showOutsideDays is equal to true, but the days after the endMonth month are not displayed in the calendar.

This behavior seems inconsistent, IMO it would feel more consistent if the day picker always showed the days or always hid the days, both the days before the startMonth month and the ones after the endMonth.
What do you think?
If it makes sense to have the "outside days" behavior the same, what would you prefer, to hide them or show them disabled?

Copy link
Owner

@gpbl gpbl left a comment

Choose a reason for hiding this comment

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

Hey, thanks for opening this PR. I missed that in our test cases.

Question: why disabling these days instead of hiding them?

More precisely, the days before the startMonth month are displayed in the calendar when showOutsideDays is equal to true, but the days after the endMonth month are not displayed in the calendar.

I believe this is because dates are listed from past to future, so the calendar just stops there, at the endMonth. But it is a good question; maybe there's a bug hidden there 👀

);

const isDisabled =
Copy link
Owner

Choose a reason for hiding this comment

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

Why disabling these days, instead of actually hiding them ? (In the line below, we calculate the isHidden for that.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me it makes sense to hide, I left this implementation in the draft because I first started with it, and then understanding better the behavior of the calendar I discovered that we have different behaviors for days before the startMonth month and the ones after the endMonth, so I wanted to check with you first what would the preferred approach if we should commit to the same behavior for both sides of the startMonth and endMonth range.

With your feedback I have enough info to finish this PR 👍
I'll update the logic and add some tests to mark the PR as ready.

@rodgobbi
Copy link
Contributor Author

I believe this is because dates are listed from past to future, so the calendar just stops there, at the endMonth. But it is a good question; maybe there's a bug hidden there 👀

Indeed, the logic responsible for that is at this line.
I tried to skip the outside days before the startMonth the same way but it breaks the calendar layout:
Skip days before startMonth issue

So the only way to not show these days is by flagging them as hidden in the useGetModifiers hook as you also suggested in the other comment.

FYI, I'll also add the logic to hide the days after the endMonth in the useGetModifiers to be safe and consistent with the same logic for the days before startMonth, because of that there's an option to refactor the getDates function and remove the maxDate because the days after the endMonth will always be hidden, but this change can affect other use cases, so it may not be worth the refactor.

@gpbl
Copy link
Owner

gpbl commented Nov 14, 2024

@rodgobbi, I added the tests for useGetModifiers in #2586. Hopefully, I haven’t undone any of your work. Please sync this branch with main, before doing more changes. Thanks a lot for your help!

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

Successfully merging this pull request may close these issues.

2 participants