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

Populate date fields with existing date if present #140

Merged
merged 1 commit into from May 17, 2017
Merged

Populate date fields with existing date if present #140

merged 1 commit into from May 17, 2017

Conversation

stuhol
Copy link

@stuhol stuhol commented Oct 23, 2016

Simple change which sets the DateTimePicker defaultDate to show any existing date stored when editing date fields.

@nodesocket
Copy link

This looks like a good change. Is this project still being maintained?

@MaxSor MaxSor merged commit 695f873 into allcount:master May 17, 2017
Copy link
Contributor

@chikh chikh left a comment

Choose a reason for hiding this comment

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

@MaxSor I think the merging was a bit premature...

@@ -154,7 +158,10 @@ allcountModule.config(["fieldRenderingServiceProvider", function (fieldRendering
locale: $locale.id.split("-")[0],
format: toMomentDateFormat(dateFormat(fieldDescription))
});
input.data("DateTimePicker").date(parseDate(controller.$viewValue) || null);
if(typeof controller.$viewValue != 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

@stuhol Why not just controller.$viewValue? Are null or 0 appropriate values here? Also parseDate already has protection against value absence: https://github.com/allcount/allcountjs/pull/140/files#diff-5be2946f54f08d277a71a12fdc822ba4R54

Copy link
Author

Choose a reason for hiding this comment

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

Yep you're right probably just controller.$viewValue would be find here.

input.data("DateTimePicker").date(parseDate(controller.$viewValue) || null);
if(typeof controller.$viewValue != 'undefined') {
input.data("DateTimePicker").date(parseDate(controller.$viewValue) || null);
input.data("DateTimePicker").defaultDate(parseDateToMoment(controller.$viewValue) || null);
Copy link
Contributor

Choose a reason for hiding this comment

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

@stuhol Could you please explain the use case of setting up defaultDate manually?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for checking this out. To be honest it's been a long time since this commit and my memory is fuzzy. I think the use case behind setting defaultDate manually was so when a user views the calendar the stored date is already selected. When defaultDate wasn't set it would default to the current date.

@@ -54,6 +54,10 @@ allcountModule.config(["fieldRenderingServiceProvider", function (fieldRendering
return s && moment(s, 'YYYY-MM-DD HH:mm:ss').toDate();
}

function parseDateToMoment(s) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@stuhol Could you please clarify why do we need parseDateToMoment (why parseDate isn't enough here)?

Copy link
Author

Choose a reason for hiding this comment

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

While testing I found that a Date wouldn't set the defaultDate correctly. However a moment would, I didn't investigate why.

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.

None yet

4 participants