gpbl / react-day-picker

DayPicker is a customizable date picker component for React. Add date pickers, calendars, and date inputs to your web applications.
https://daypicker.dev
MIT License
6.03k stars 722 forks source link

Bug: selected range is including disabled days #1885

Closed gpbl closed 2 months ago

gpbl commented 1 year ago

As discussed in https://github.com/gpbl/react-day-picker/pull/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

https://github.com/gpbl/react-day-picker/assets/120693/7a990678-4ec0-4665-8b70-f652eaf3b107

Workaround

A workaround of this issue is shown in the CodeSandbox above:

import { isAfter, isBefore } from "date-fns";
import React from "react";
import { DateRange, DayPicker } from "react-day-picker";
import "react-day-picker/dist/style.css";

function rangeIncludeDate(range: DateRange, date: Date) {
  return Boolean(
    range.from &&
      range.to &&
      isAfter(date, range.from) &&
      isBefore(date, range.to)
  );
}

const disabledDate = new Date();

export default function App() {
  const [selectedRange, setSelectedRange] = React.useState<
    DateRange | undefined
  >();

  const handleSelect = (range: DateRange | undefined, selectedDate: Date) => {
    setSelectedRange(() => {
      if (range && rangeIncludeDate(range, disabledDate)) {
        if (range.from && isBefore(selectedDate, disabledDate)) {
          return { from: range.from, to: undefined };
        }
        return { from: range.to, to: undefined };
      }
      return range;
    });
  };

  return (
    <DayPicker
      mode="range"
      selected={selectedRange}
      onSelect={handleSelect}
      disabled={disabledDate}
      footer={<pre>{JSON.stringify(selectedRange, null, 2)}</pre>}
    />
  );
}
brandishcode commented 1 year ago

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?

filipc30 commented 1 year ago

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.
Also I have randomly found this issue after I've already solved it customly in my project and didn't even realize this package is still being developed, I just thought it is a bug that won't be fixed. πŸ˜†

I also think that selecting range with some dates disabled shouldn't just reset the values, but also have default error handling, as well as if you select for example date 24th (as a {range.from} Date), and 25th is disabled, all dates after 25th should get disabled too, as this is much better ux/ui behaviour, see more in my example:

https://github.com/gpbl/react-day-picker/assets/101244540/590504c8-8d66-4404-a480-1aa772f71540

brandishcode commented 1 year ago

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 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 from all other dates that cannot be selected as to becomes disabled (here).

The error handling does help with ux/ui.

gpbl commented 1 year ago

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.

gpbl commented 1 year ago

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?

@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! πŸ€–

brandishcode commented 1 year ago

@gpbl

Thanks for clearing up how to contribute here.

My suggestion is the following for range mode:

This approach in my opinion is much more clearer in user experience. Let me know what you guys think.

stabildev commented 9 months ago

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. Also I have randomly found this issue after I've already solved it customly in my project and didn't even realize this package is still being developed, I just thought it is a bug that won't be fixed. πŸ˜†

I also think that selecting range with some dates disabled shouldn't just reset the values, but also have default error handling, as well as if you select for example date 24th (as a {range.from} Date), and 25th is disabled, all dates after 25th should get disabled too, as this is much better ux/ui behaviour, see more in my example:

Screen.Recording.2023-09-23.at.8.58.39.AM.mov

@fcablik Any chance you're willing to share your implementation?

crazyyi commented 8 months ago

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.

sparanoid commented 2 months ago

@gpbl After upgrading to v9 this has become an issue in my use case. I have a list of live streams, and I have disabled the days on the calendar that do not have any streams. However, users might want to select a range from the previous week to view weekly stats, which is currently not possible in v9 due to the disabled days.

The behavior in v8 is intended in my case. An option like limitSelectRangeWithoutDisabledDays https://github.com/gpbl/react-day-picker/pull/1878 would be great.

gpbl commented 2 months ago

@sparanoid thanks for your feedback. I think it is important to restore the original behavior and make the new one as optional.

See https://github.com/gpbl/react-day-picker/pull/2290