mymth / vanillajs-datepicker

A vanilla JavaScript remake of bootstrap-datepicker for Bulma and other CSS frameworks
MIT License
752 stars 155 forks source link

Month / Year view : minDate and maxDate blocked #56

Closed steve-ks closed 2 years ago

steve-ks commented 3 years ago

Hey there I recently thrown out all of jQuery / bootstrap of a project and used your plugin as a replacement. Works flawlessly and mostly perfect. Our setup is a daterange picker for different view levels in date, month or year view, so users can live-edit by clicking between their view level.

One thing we recognized during testing is that when switching to either day or month view the range-dates set by minDate and maxDate are not selectable anymore. I tracked the problem down to the date validation and parsing based on the format.

What I found out (I do not open a PR but simply describe here as it is quicker for me for the moment):

In Datepicker.js following lines do check if we are in month / year view. The problem comes when we set new options by the API method setOptions and internly the processOptions / validateDate functions parse the Dates. https://github.com/mymth/vanillajs-datepicker/blob/9de5051545626b116db3d213b0b1e429f296967c/js/Datepicker.js#L36-L59

What I pass in to the options:

minDate: "2019-10",
maxDate: "2021-03",
format: "yyyy-mm"
...

For todays date (2021-03-25), I added a console-log at following position: https://github.com/mymth/vanillajs-datepicker/blob/9de5051545626b116db3d213b0b1e429f296967c/js/options/processOptions.js#L24-L28 retrieving following output dates - time values rewritten in readable format (here only the max-date):

{
  value: "2021-03",
  format: "yyyy-mm",
  origValue: "2021-03-25"
  returnValue: "2021-03-01"
}

The return value for the max-date is not useful, as it reflects the beginning of the month, not the ending. What struggles me more is that for the rangeEnd range-check now the maxDate is correctly set to the last day of the month but checked against the first day of the month, which then leads to a blocked selectable month in the picker (when clicking, nothing happens).

The same problem comes when using year format, the max-range year is not selectable.

I guess there are now several possible fixes, one way I fixed the problem locally is by modifying the min- and max-date ranges in Datepicker.js (the lines referenced at the beginning) as following:

  let newDates = inputDates.reduce((dates, dt) => {
    let date = parseDate(dt, config.format, config.locale);
    if (date === undefined) {
      return dates;
    }
    let minDate = config.minDate
    let maxDate = config.maxDate
    if (config.pickLevel > 0) {
      // adjust to 1st of the month/Jan 1st of the year
      // or to the last day of the monh/Dec 31st of the year if the datepicker
      // is the range-end picker of a rangepicker
      const dt = new Date(date);
      if (config.pickLevel === 1) {
        // MONTH
        date = rangeEnd
          ? dt.setMonth(dt.getMonth() + 1, 0) // set to last day of month
          : dt.setDate(1); // set to first day of month
      } else {
        // YEAR
        date = rangeEnd
          ? dt.setFullYear(dt.getFullYear() + 1, 0, 0) // set to last day of year
          : dt.setMonth(0, 1); // set to jan 1st
      }
      // We need to also adjust the month / year view for min and max dates
      maxDate = config.pickLevel === 1
        ? (new Date(maxDate)).setMonth(dt.getMonth() + 1, 0) // set to last day of month
        : (new Date(maxDate)).setFullYear(dt.getFullYear() + 1, 0, 0) // set to last day of year
      minDate = config.pickLevel === 1
        ? (new Date(minDate)).setDate(1) // set to first day of month
        : (new Date(minDate)).setMonth(0, 1) // set to jan 1st
    }

By updating the min/max ranges now the isInRange method correctly allows the selection of the upper range month / year, in my example 2021-03.

Hope this helps. Maybe I open a PR later, but feel free to do it yourself.

mymth commented 3 years ago

I'm sorry but I'm a bit unclear about your setup. Is it something like this? https://codepen.io/mymth/pen/YzNWOmE

The result you got from the console-log is hard to guess why, but one possibility that can happen is when a Date object of 2021-03-01 is passed to the value argument of validateDate(). So I'm wondering if the value of maxDate you passed to setOptions() was not a string '2021-03' but a Date object new Date(2021, 2).

I don't think it's a good manner to manipulate config values inside the functions that refer to them. Those functions should run strictly under the condition defined by config values. Making such a bypass route can confuse other people who need to trace the code for other purpose.

steve-ks commented 3 years ago

I've updated the pen to show the problem (running with daterangepicker): https://codepen.io/steve-ks/pen/NWdaNLR

When switching to month-mode, march is not selectable at the from-field. april thus is not selectable in the to-field. Yet both fields are selectable vice-versa (march in to and april in from) in this mode, even together which leads to a range of 2021-04 to 2021-03 (negative date range).

In year mode the problem is even worse as we can neither select a year nor get rid of the datepicker when opened. We need to switch back to day-mode to get any date selected.

If this is a problem of configuration (I did not pass in any date object but strings) than the documentation should somehow give a hint, at least. Yet for me this seems more like a bug in code. With the updated DatePicker object as suggested by my inital post the problem disappears.

mymth commented 3 years ago

Thank you for updating the pen. Now I see the real problem of the issue. And this is a quick solution without patching the library. https://codepen.io/mymth/pen/abpqLjp

The real problem is that the date values set to the minDate/maxDate config values are inappropriate in most cases when the pickLevel option is not 0 and it's impossible for users to set appropriate minDate/maxDate config values via date string if the format option doesn't have the day part.

You are right that this is a bug. (Don't get me wrong. Passing Date object is fully supported. I just said about the case validateDate() can result like your console-log) But since this is a bug of setting configuration values, the right place to writing a fix is not Datepicker.js but processOptions.js. Your patch doesn't fix the real problem. Instead, it creates an exceptional case that allows setDate() to break the rule by tricking the method's check logic. If this way of patching is repeated, the program gradually becomes difficult to trace. (This is what I saw in bootstrap-datepicker's code and the reason why I decided to rewrite the code from scratch.)

So, I'm sorry but for the above reasons, I'll turn your PR down and fix the bug in the next update.

steve-ks commented 3 years ago

Your patch doesn't fix the real problem. Instead, it creates an exceptional case that allows setDate() to break the rule by tricking the method's check logic. If this way of patching is repeated, the program gradually becomes difficult to trace. (This is what I saw in bootstrap-datepicker's code and the reason why I decided to rewrite the code from scratch.)

This is indeed a tricked workaround instead of a clean bugfix. As my bugfix was mostly a quick and dirty solution as I don't have full insight into your library I am glad to get a real solution instead of my PR anyway.

Thanks for rethinking and keeping the code clean. Looking forward to your solution in the next update then.

steve-ks commented 3 years ago

In the current version this problem seems to be still there. Is there a release with a fix planned as mentioned ?

mymth commented 3 years ago

Sorry for taking long. The fix will definitely be included in the next release.