-
-
Notifications
You must be signed in to change notification settings - Fork 696
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
Bug: selected range is including disabled days #1885
Comments
Hi, I am a bit new to open source development. If I wanted to develop bug fix for this, should I ask permission from maintainer first? |
I think you don't have to seek permission, just open a pull request with your fixes/changes. I also think that selecting range with some dates disabled shouldn't just reset the values, but also have default error handling, Screen.Recording.2023-09-23.at.8.58.39.AM.mov |
I see! Good to know. Same for me I was confused at first I was able to select disabled days. I actually created a fix for this, the might be similar to what you said. When you select a The error handling does help with ux/ui. |
Hi @fcablik @trabeast thanks for your interesting points. I understand there are use cases that are missing in the range selection mode. Range selections requires understanding some tricky user interactions and edge case. I expect our implementation to miss some of them. For this reason, we support custom selection: basically, you decide by yourself what to do when the user picks a day, according to the dates already stored in your component's state. |
@trabeast no need of permissions! Thanks for asking :) To avoid to waste our time, I encourage first a async discussion about the problem and some possible solutions. You are free to open a PR to propose some changes - the discussion could have place there. For this case, I'd start by writing down some requirements to agree first how range selections should work. Once finished, these requirements can be translated into tests and code changes. Examples:
Thanks! 🤖 |
Thanks for clearing up how to contribute here. My suggestion is the following for
This approach in my opinion is much more clearer in user experience. Let me know what you guys think. |
@fcablik Any chance you're willing to share your implementation? |
I found the selection logic on Airbnb easy to understand and not too complicated. When you select the start/from date, if there are disabled days after this date, any date after the disabled/reserved days would be disabled too. And if the user wants to select a new range she needs to click the "clear date" to reset the selection. |
As discussed in #1878, range selections may include disabled day. Instead, selecting a range containing a disabled day should reset the range as in this example:
https://codesandbox.io/s/recursing-bouman-r7sz4n?file=/src/App.tsx
Screen.Recording.2023-08-17.at.15.44.15.mov
Workaround
A workaround of this issue is shown in the CodeSandbox above:
The text was updated successfully, but these errors were encountered: