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(datetime):change default year when max value is defined #7933

Closed
wants to merge 2 commits into from

Conversation

ogoguel
Copy link

@ogoguel ogoguel commented Aug 31, 2016

cf. issue #7932

@ogoguel
Copy link
Author

ogoguel commented Sep 1, 2016

It is already the case.

This PR fixes the validate part of the datepicker to be sure that that the date used during the validation is within the min/max range so months and/or days are not disabled when the datepicker is being presented to the user.

It matches the current implementation where the max date is being used for pre-selection if present ( as there's no value selected, the datepicker will use the first available year, hence the max one)

Having the developer being able to set the default date could be a nice improvement though

@ogoguel
Copy link
Author

ogoguel commented Sep 1, 2016

Set default date to max to be sure a proper date is always presented to the user (cf. #7946)

@manucorporat
Copy link
Contributor

manucorporat commented Oct 7, 2016

@ogoguel not sure of this:

+    let selectedYear = this._max ? this._max.year : today.getFullYear();

Imo it makes more sense something like:

let selectedYear = today.getFullYear();
if ( this._max < selectedYear) {
  selectedYear = this._max;
}

even better with a functional approach:

let selectedYear = Math.min(this._max, Math.max(this._min, today.getFullYear()));

am I missing something?

NOTE, I used pseudocode (didn't tested it)

@ogoguel
Copy link
Author

ogoguel commented Oct 7, 2016

How about if max is undefined ? in that case, selectedYear should be set to the currentYear, and I don't know how min/max will behave with undefined value

@jgw96
Copy link
Contributor

jgw96 commented Jan 31, 2017

Hello, thanks for opening a PR with us, would you be able to fix the merge conflicts here please so we can continue to review.

@jgw96 jgw96 added the needs: reply the issue needs a response from the user label Jan 31, 2017
@ogoguel
Copy link
Author

ogoguel commented Feb 1, 2017

I did some quick test and it appears to be fixed in the 2.0.0 rc5 branch. However, if there are multiple date pickers in the same page, the max date seems to be ignored. I will investigate

@manucorporat manucorporat modified the milestone: 2.3.0 Mar 4, 2017
@manucorporat
Copy link
Contributor

I think it was definitively fixed in 2.3, feel free to reopen if you confirm I am wrong

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: reply the issue needs a response from the user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants