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

Support for custom "date_format" and "time_format" arguments #498

Closed
wants to merge 1 commit into from

Conversation

mweimerskirch
Copy link

Support for custom "date_format" and "time_format" arguments for the text_date, text_date_timestamp and text_datetime_timestamp fields.

Original fixes by @yivi, based on #318

I removed all the unrelated changes, and added support for the time picker. To make merging easier, the pull request also doesn't include a minified JS file.

…text_date, text_date_timestamp and text_datetime_timestamp fields
@jtsternberg
Copy link
Member

Why do you believe your PR is a better option than @yivi's #318? What do you consider to be "unrelated changes"?

@mweimerskirch
Copy link
Author

I downloaded the patch file for #318 directly from GitHub because I didn't want to pull in 23 individual commits and there were hundreds of whitespace changes as well as changes in a translation file: https://github.com/WebDevStudios/CMB2/pull/318.patch
But just now after reading your message I noticed that those changes are not visible when viewing the diff online. Seems to be a bug on GitHub, so I take the part about the "unrelated changes" back.

@yivi
Copy link
Contributor

yivi commented Oct 19, 2015

yivi here.

I see that in your version you are are back to using DateTime::createFromFormat, whereas we had a workaround for PHP versions < 5.3.

Also, my work continued in #446, on a different branch that @jtsternberg created to carry on this work. I updated it a couple weeks ago, and it does have support for time_picker as well...

I think I messed up with the PRs, by opening a new one instead of continuing work in #318, but I can merge trunk into #446 again to see if everything keeps working as intended.

Regards.

@jtsternberg
Copy link
Member

@yivi we're so close. Thanks for your efforts. Let me know which (#318 or #446) I should be testing against. For now, i've merged trunk into the WebDevStudios:yivi-date-time-picker-fixes branch so it is up-to-date.

@mweimerskirch
Copy link
Author

@yivi Could you squash the commits for the current pull request? That way I could test it as well and let you know if any of my use cases fail.

@yivi
Copy link
Contributor

yivi commented Oct 20, 2015

@jtsternberg , #446 is the current one.
I'll update it to trunk this afternoon.

@jtsternberg
Copy link
Member

This should be good in trunk. Huge thanks for @yivi's work. Please test.

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

3 participants