Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

Added min and max options #124

Open
wants to merge 5 commits into
base: gh-pages
Choose a base branch
from
Open

Added min and max options #124

wants to merge 5 commits into from

Conversation

stap123
Copy link

@stap123 stap123 commented Jul 3, 2018

Added options to allow a min and max to be specified for the time.
Only works in 24 hour mode

Adam Stapleton added 3 commits July 3, 2018 23:30
Added options to allow a min and max to be specified for the time.
Only works in 24 hour mode
@DervishMarauder
Copy link

Hi Adam,

Just been using this PR as part of a project that I'm working on, was glad to see that somebody had already thought of and had done work towards this requirement. Upon using this PR I found an issue with the minute validation, it appears that the minutes of the min hour are validated against the minutes of the max hour.

I've made some minor changes to fix this and wondered if you would add them to the PR incase anybody else has this issue?

From line 593:

if (this.min || this.max) {
    var ticks = this.minutesView.find('div.clockpicker-tick');
    var tick,
        tickValue;

    if (!this.hours) {
        this.hours = parseInt(this.spanHours.html());
    }
    if (!this.minutes) {
        this.minutes = parseInt(this.spanMinutes.html());
    }
    if (!isHours && (this.hours === this.min[0])) {
        // mark items as invalid for minutes if min hour applies
        for (let i = 0; i < ticks.length; i++) {
            tick = $(ticks[i]);
            tickValue = parseInt(tick.html());
            if (tickValue < this.min[1]) {
                tick.addClass('invalid');
            }
        }
    }
    else if (!isHours && (this.hours === this.max[0])) {
        // mark items as invalid for minutes if max hour applies
        for (let i = 0; i < ticks.length; i++) {
            tick = $(ticks[i]);
            tickValue = parseInt(tick.html());
            if (tickValue > this.max[1]) {
                tick.addClass('invalid');
            }
        }
    }
    else {
        for (let i = 0; i < ticks.length; i++) {
            tick = $(ticks[i]);
            tick.removeClass('invalid');
        }
    }
}

@stap123
Copy link
Author

stap123 commented Oct 24, 2018

Hi @DervishMarauder

Are you sure that is the case, the code you've provided seems to do exactly the same apart from it is separated into more lines?

The content of

if (!isHours && (this.hours === this.min[0])) {
        // mark items as invalid for minutes if min hour applies
        for (let i = 0; i < ticks.length; i++) {
            tick = $(ticks[i]);
            tickValue = parseInt(tick.html());
            if (tickValue < this.min[1]) {
                tick.addClass('invalid');
            }
        }
    }
    else if (!isHours && (this.hours === this.max[0])) {
        // mark items as invalid for minutes if max hour applies
        for (let i = 0; i < ticks.length; i++) {
            tick = $(ticks[i]);
            tickValue = parseInt(tick.html());
            if (tickValue > this.max[1]) {
                tick.addClass('invalid');
            }
        }
    }

Are exactly the same and as such can be combined into

if (!isHours && ((this.hours === this.min[0]) || (this.hours === this.max[0]))) {
        // mark items as invalid for minutes if min hour applies
        for (let i = 0; i < ticks.length; i++) {
            tick = $(ticks[i]);
            tickValue = parseInt(tick.html());
            if (tickValue < this.min[1] || tickValue > this.max[1]) {
                tick.addClass('invalid');
            }
        }
    }

I don't think your code does anything differently to the current content apart from the below.

As a result of looking at this I have noticed there is at least one bug in the code with incorrect use of the '=' instead of '==='. Specifically on line 602:

if (!isHours && ((this.hours = this.min[0]) || (this.hours = this.max[0]))) {

I think should be:

if (!isHours && ((this.hours === this.min[0]) || (this.hours === this.max[0]))) {

I've updated.

I'm not sure if that fixes your issue or not however. Can you test with the updated code and if that doesn't work elaborate on your issue and provide me a simple repro for it? I can then have a look and try and fix the issue for you?

If I've misunderstood something, happy to be corrected 😃

@Aphyxia
Copy link

Aphyxia commented Nov 14, 2018

I came across this issue also that the minimum hour only allowed 00 minutes to be picked like the last hour only allows 00. DervishMarauders code helped, the equality operator fix didn't.

Updates made as per suggestion from @DervishMarauder

Co-Authored-By: Matthew Southern <[email protected]>
@stap123
Copy link
Author

stap123 commented Nov 15, 2018

@Aphyxia @DervishMarauder I have updated to include the code provided by @DervishMarauder as it would seem that it solves the issues described by you both. Hopefully that's useful for someone else in the future.

I am still unsure as to the difference if I'm honest, if someone feels like explaining I would appreciate it.

@Aphyxia
Copy link

Aphyxia commented Nov 15, 2018

Well, the problem is that when the hour is equal to the minimum hour, only the minute limitation of the minimum hour should be taken into account, therefore the statement if (tickValue < this.min[1] || tickValue > this.max[1]) can not be used for the minimum hour, since it resolves as true for every minute of the minimum hour if it's larger than the set minutes of the maximum time. Since mine was 18:00, the only allowed minutes for the minimum time also became 00. The same bug could also have happened for maximum time but would be less common, considering the usual use cases.

@stap123
Copy link
Author

stap123 commented Nov 15, 2018

@Aphyxia That makes sense, thanks for explaining 👍

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

Successfully merging this pull request may close these issues.

3 participants