setting a Calendar minDate that is later than the currently set maxDate fails silently

[h0lg]

Hey team!

I've been learning about fomantic a bit and have come across some unexpected behavior using the Calendar module:

Given I have a calendar widget with minDate and maxDate set, when I set a new minDate that is later than the current maxDate I would like to

• either get an exception saying the minDate must not be later than the current maxDate
• or the new minDate to be accepted and the maxDate to be unset (i.e. set to null).

What I experience instead is that the setting of the new minDate silently fails. This caused me some headache today until I realized I can work around this by first setting a new maxDate (or unsetting it). Play with my fiddle.

Other than that: Good job, keep it up! Cheers!

[lubber-de]

This is already fetched and a warning is put into the console when debug+verbose is set to true

$('#standard_calendar').calendar({
  debug:true, 
  verbose:true
});

See your adjusted jsfiddle here https://jsfiddle.net/ytj3f4L6/

[h0lg]

@lubber-de I see, thanks for the heads up! I'll remember to look for and set these flags while I'm exploring the API.

This behavior still feels a bit odd - I would normally expect either my later commands to overrule my previous ones or to get a noticeable complaint (i.e. exception) when my command is ignored. Knowing about and remembering those flags makes the learning curve just a bit steeper for fomantic noobs like me. But if this kind of behavior (i.e. silent failing unless in debug+verbose mode) is in line with the philosophy of the framework, I'll accept that and move on.

On a way less important note, there's a typo in that warning: Calendar: Unable to set minDate variable bigger that than maxDate variable

[lubber-de]

@forger Agreed. I prepared a PR #1366 so

• this behavior is more clear (error logging instead of verbose)
• a new optional setting will auto adjust the opposite setting so the range stays valid
• the typo in the message is fixed

See your adjusted fiddle here https://jsfiddle.net/k386q42s/1/

[h0lg]

@lubber-de Thanks for your continuing support :) I have seen your PR and am asking myself whether anybody not familiar with this particular problem will understand the new setting autoAdjustDateEdges: false [true | false] you've added. It didn't seem to be obvious to @exoego.

What do you think about something like this instead?

validateMinMax: 'debug-warning' ['debug-warning' | 'throw' | 'reset-invalid-to-same' | 'reset-invalid-to-null' | 'off'] - determines the calendar's reaction to setting a new minDate later than the current maxDate or a new maxDate earlier than the current minDate

I've tried to be a bit more explicit in the naming here and listed some sensible options for treating this issue that come to mind: debug-warning: logs a warning to the console if debug: true and verbose: true (current behavior) throw: throws an Exception reset-invalid-to-same: resets the current invalid date limit to the same value as the new date limit reset-invalid-to-null: unset the current invalid date limit (i.e. resets it to null) off: disable min/max date validation

Notice that I've also kept debug-warning as the default, to stay backwards-compatible like your PR. But I don't think that staying backwards-compatible to the current (IMHO odd) behavior is necessarily a good thing. In this case, I'd rather break backwards-compatibility because for me, the option debug-warning is not really a viable one. As a fomantic noob, I'd stay away from options I don't understand and would instead go with the defaults hoping that they're sensible.

And at that point I'd question whether the widget really needs a new option that's rather hard to explain instead of just having its default behavior changed and being opinionated about how to handle this corner case.

[lubber-de]

Basically we should keep this as easy and small as possible inside the framework not to increase the logic unnecessarily. Remember the current workaround still is to set the maxDate before setting the mindate or even make sure in your own code to check for valid ranges before even using the "set minDate" behavior at all. In that case we won't even need a PR at all 😉

'debug-warning' | 'throw'

I would not differentiate between debug or throw, because it's quite obvious that a min date cannot be set after a maxdate, so it's wrong in any case and should be directly put into the console

'reset-invalid-to-same' | 'reset-invalid-to-null']

I would reduce this to 'same' and 'null'...personal taste, let's see what others say @fomantic/maintainers

I don't think that staying backwards-compatible to the current (IMHO odd) behavior is necessarily a good thing. In this case, I'd rather break backwards-compatibility because for me, the option debug-warning is not really a viable one.

The reason for staying backwards compatible is to be able to merge a PR within the 2.8.x branch. Any kind of breaking change has to wait until 2.9

[h0lg]

The reason for staying backwards compatible is to be able to merge a PR within the 2.8.x branch. Any kind of breaking change has to wait until 2.9

I understand and figured as much.

Remember the current workaround

we should keep this as easy and small as possible

Agreed! In short: Remembering the current workaround ;) I'm happy to wait until you've figured out a consistent and easy to understand solution for 2.9 rather than introducing a new option in 2.8.x that's rather hard to explain and wrap your head around.

I think both your setting and my alternative have the same problem: They're hard to communicate and understand for someone not familiar with the problem. Mind you I only tried for a more explicit alternative and don't really favor it. Even while writing down my idea I realized that it's too complicated to explain it unambiguosly. I.e. people will stumble across this in the API and be like WTF.

The more I think about it, the more attractive a change in default behavior becomes. As a developer, I want to get notified if my command is invalid in the current state of the widget so that I can work around it. And I want this to happen reliably - i.e. through an exception, not a console warning that I get if I have set the debug and verbose flags.

If however, my command is accepted and "just" has the side effect of resetting/unsetting the opposite (now invalid) range limit, I would consider that acceptable behavior and would learn to work with it. I kind of like that option, because it makes the correct implementation simple and the side effect is rather easy to communicate. In my particular scenario, the resetting/unsetting would work just fine because I'd reset the opposite limit in the next statement anyway:

//SCENARIO: currentMinDate < currentMaxDate && currentMaxDate < newMinDate && newMinDate < newMaxDate
$widget.calendar('set minDate', newMinDate); //currently fails silently because currentMaxDate < newMinDate
$widget.calendar('set maxDate', newMaxDate); //would set maxDate to newMaxDate if above statement set it to newMinDate or null instead of failing

If the validation threw an exception, a correct implementation would obviously look different - I'd have to check whether currentMaxDate < newMinDate and depending on that either set newMinDate or newMaxDate first - which is the same implementation that the current behavior requires.

//working around invalid command
if (currentMaxDate < newMinDate) {
   $widget.calendar('set maxDate', newMaxDate);
   $widget.calendar('set minDate', newMinDate);
} else {
   $widget.calendar('set minDate', newMinDate);
   $widget.calendar('set maxDate', newMaxDate);
}

Realizing that the real problem here is that because the setting of these options is being validated individually, you never know which one you can set first without violating the minDate <= maxDate rule, you might also want to take a completely different approach and either allow disabling validation or create a new behavior to set minDate and maxDate simultaneously and only validate after both have been set - just a thought.

$widget.calendar('set validateMinMaxSequence', false); // new setting? [true | false]
$widget.calendar('set minDate', newMinDate);
$widget.calendar('set maxDate', newMaxDate);

or

$widget.calendar('set minMaxDates', newMinDate, newMaxDate); //new behavior?

[h0lg]

Slightly different issue, but related to this one: Setting the maxDate to null doesn't seem to have an effect on validation. That is, the minDate validation will still remember the (previous) maxDate. See this fiddle.

[h0lg]

Setting the maxDate to null doesn't seem to have an effect on validation. That is, the minDate validation will still remember the (previous) maxDate.

There seems to be a bigger issue with resetting the date boundaries. The popup doesn't update accordingly. I've added a dedicated bug report: #1776