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

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

Open
4 tasks done
Banner-Keith opened this issue Jan 15, 2024 · 2 comments
Open
4 tasks done
Labels
State: Triage Tickets that need to be triaged. Type: Bug

Comments

@Banner-Keith
Copy link

Prerequisites

  • I have searched for duplicate or closed issues.
  • I have validated my setup against the latest version on StackBlitz.
  • I have read the contributing guidelines.
  • I understand that only sponsored issues will be worked on and that if I wish to sponsor an issue, I will contact the owner FIRST.

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
Copy link
Owner

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.

@Banner-Keith
Copy link
Author

@Eonasdan I have submitted a PR for the doc change. I am still potentially interested in the custom validation being an option at some future date. I would be willing to do a PR for that if you want me to. Just let me know how you'd like it to be designed. It may be a bit before I can get to that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
State: Triage Tickets that need to be triaged. Type: Bug
Projects
Status: 🆕 New
Development

No branches or pull requests

2 participants