Eonasdan / tempus-dominus

A powerful Date/time picker widget.
https://getdatepicker.com
MIT License
7.16k stars 4.41k forks source link

Difference between documentation and actual behavior of restriction options for dates #2891

Open Banner-Keith opened 5 months ago

Banner-Keith commented 5 months ago

Prerequisites

Describe the issue

This isn't really a bug. Perhaps an ask for a bit of clarification in the documentation. And also potentially a small configuration feature that I would code and build tests for myself if you are open to it. I do not want to give you more work to do. If you are not open to having other people add features that's totally fine.

Explanation:

The docs say that both enabledDates and disabledDates take precedence over min and max. The impression I got from that was that adding enabled or disabled dates would override the min and max. Which is not really how it works.

In practice, and also from looking at the v6 code, there's really a different pattern at play here.

  1. The date is checked against the disabled dates. If a date is disabled it will be disabled. This makes sense.
  2. The date is checked against the enabled dates. If there are any enabled dates, all other dates will be disabled no matter what.
  3. If the date is in the enabled dates list it can still be disabled if the min and max apply.

This is a bit confusing because my assumption from the docs was that if I added something to enabledDates it would show up even if min and max excluded that date.

Documentation ask

I do not think it makes sense to change any of this behavior as it would be a breaking change and affect people already using this as is. So maybe the docs could mention that if any of the four date restrictions make a date invalid the date will be disabled, and that dates will only be available if they meet all four criteria. May even need to document the order of disabled being checked first then disabled.

Potential minor new feature contribution by me through PR

I did see that in issue #2794 you were working on custom validation hooks which would totally solve my problem but I wasn't sure if 7.0 is still going to happen. If 7.0 is coming on some kind of timeline I can wait for that and implement my solution using that.

Since none of the existing behavior should change, I thought of adding a new option to cover my use case. My use case is that I have a date range that is valid, but if an existing record is pulled up I don't want to force a new date to be selected. The current old one is still valid even if it is not in the range. So I thought I could do a range, and then add the current value to the enabledDates.

Could we add a new option like enabledDatesOverride? It would enable the date even if all four of the other options showed it as disabled.

If you are okay with that idea we'd just need to check that option first before all of the other ones in the isValid function https://github.com/Eonasdan/tempus-dominus/blob/5f79399779918fd588ae1f4d598b3de30998ca00/src/js/validation.ts#L21C63-L21C63

I would be happy to open a PR with the option, any tests you'd like, documentation etc.

StackBlitz fork

Disabled Date and min max. Works pretty much as I'd expect. https://tempus-dominus-v6-simple-setup-tdutmh.stackblitz.io

Enabled Date and min max. Confused me initially because I had assumed that enabled dates override min and max but nothing ends up enabled at all because I don't have any enabled dates within the valid date range. https://tempus-dominus-v6-simple-setup-2qzjpf.stackblitz.io

What operating system(s) are you seeing the problem on?

Windows, Android

What browser(s) are you seeing the problem on?

Chrome, Microsoft Edge

What version of are you using? You can find this information from the sample StackBlitz.

6.7.16

What your browser's locale? You can find this information from the sample StackBlitz.

en-US

Eonasdan commented 5 months ago

Thank you for your detailed report.

Unless I get some corporate sponsors, v7 is most likely not going to happen.

For the validation hooks, I was thinking that I would add an option that was a function and call that as the first check.

isValid(targetDate: DateTime, granularity?: Unit): boolean {
    if( ...customValidation is not null and !customValidation(granularity, targetDate) {
        return false
    }

It could be expanded to include a pre and post validation option so that if a dev wanted to let the built in validation happen first and then do a final validation at the end.

Perhaps the custom validation function also takes a this so that functions like _enabledDisabledDatesIsValid would be available to use.

I'm willing to accept PR changes for the docs if you want to make that more clear. If you're interested in a PR for the custom validation hooks then let's iron it out more first. In either case we can do this against the master (v6) branch.