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

Fix #7381 (Calendar incorrect time validations); combine into 1 validation method #7382

Closed
wants to merge 2 commits into from

Conversation

polushinmk
Copy link
Contributor

###Defect Fixes
#7381

I suggest to combine validateHour, validateMinute and validateSecond into 1 validation method = validateTime
this should fix all incorrect time validations

@polushinmk polushinmk changed the title Fix #7381 Fix #7381 (Calendar incorrect time validations); combine into 1 validation method Oct 28, 2019
let valueDateString = value ? value.toDateString() : null;

if (this.minDate && valueDateString && this.minDate.toDateString() === valueDateString) {
if (this.minDate.getHours() > hour) {
Copy link

@thomasesh thomasesh Oct 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't take into account if the hour is in 12 format. The minDate and maxDate are in 24 hour format.

I am using both date and time selection.

Here is my attempt to fix: (this is in .js, not .ts format (sorry))

Change validateTime to take " this.pm" argument everywhere in your commit.
change validateTime to be:

Calendar.prototype.validateTime = function (hour, minute, second, pm) {
       var value = this.value;
       var convertedHour = this.convertTo24Hour(hour, pm);
       if (this.isRangeSelection()) {
           value = this.value[1] || this.value[0];
       }
       if (this.isMultipleSelection()) {
           value = this.value[this.value.length - 1];
       }
       var valueDateString = value ? value.toDateString() : null;
       if (this.minDate && valueDateString && this.minDate.toDateString() === valueDateString) {
           if (this.minDate.getHours() > convertedHour) {
               return false;
           }
           if (this.minDate.getHours() === convertedHour) {
               if (this.minDate.getMinutes() > minute) {
                   return false;
               }
               if (this.minDate.getMinutes() === minute) {
                   if (this.minDate.getSeconds() > second) {
                       return false;
                   }
               }
           }
       }

       if (this.maxDate && valueDateString && this.maxDate.toDateString() === valueDateString) {
           if (this.maxDate.getHours() < convertedHour) {
               return false;
           }
           if (this.maxDate.getHours() === convertedHour) {
               if (this.maxDate.getMinutes() < minute) {
                   return false;
               }
               if (this.maxDate.getMinutes() === minute) {
                 if (this.maxDate.getSeconds() < second) {
                     return false;
                 }
               }
           }
       }
       return true;
   };

    Calendar.prototype.convertTo24Hour = function (hours, pm) {
      if (this.hourFormat == '24') {
        return hours;
      }
      if (pm) {
        hours = hours + 12;
        return (hours > 23 ) ? 0 : hours;
      }
      return (hours > 11) ? 0 : hours;
    };

and change toggleAMPM to be:

 Calendar.prototype.toggleAMPM = function (event) {
        if (this.validateTime(this.currentHour, this.currentMinute, this.currentSecond, !this.pm)) {
          this.pm = !this.pm;
          this.updateTime();
        }
        event.preventDefault();
    };

@cagataycivici
Copy link
Member

See #8290

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants