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 so datetime combo field respects date-format and time format a set up by the user #446

Merged
merged 31 commits into from Nov 23, 2015
Merged

Conversation

yivi
Copy link
Contributor

@yivi yivi commented Aug 29, 2015

I still have to review the datetime-timezone field.

A couple things:
Since in CMB_Field.php:630 you have
return date( stripslashes( $this->args( $format ) ), $meta_value );
users wont be able to use escape characters in their format strings. (E.g.: 'd \d\e m \d\e Y', or any other combinations).

Either that should be reflected in the docs, or that particular stripslashes should go... :P

I edited the wiki to reflect that there are no longer separate "text_date" and "text_date_timestamp". But I guess we should also put there the acceptable characters for the format string (d, j, z, m, n, y, and Y, instead of the whole date() range).

I'll try to finish up the timezone combo later today.

Regards!

yivi and others added 28 commits May 8, 2015 09:35
…tialized with its own date-format

The js initializer reads data-datepicker to set each datepicker properly
Conflicts:
	includes/CMB2_Sanitize.php
	includes/CMB2_Types.php
	js/cmb2.min.js
	languages/cmb2-es_ES.mo
	languages/cmb2-es_ES.po
Conflicts:
	js/cmb2.min.js
	languages/cmb2-es_ES.mo
	languages/cmb2-es_ES.po
@yivi
Copy link
Contributor Author

yivi commented Aug 30, 2015

Timezone combo done as well.
As an aside... for date/time combo (no timezone) we are saving an array with 'date' and 'time' as keys, while just saving a timestamp should be sufficient (and maybe cleaner?)

Regards, hope this helps.

Conflicts:
	includes/CMB2_Types.php
	includes/CMB2_Utils.php
	js/cmb2.min.js
@yivi
Copy link
Contributor Author

yivi commented Oct 21, 2015

@jtsternberg, this PR has current trunk merged in, and I have tested it and have it working for all date/time types I'm aware of.

@@ -212,10 +225,12 @@ public function text_datetime_timestamp( $repeat = false ) {
return $repeat_value;
}

$this->value = strtotime( $this->value['date'] . ' ' . $this->value['time'] );
if ( is_array( $this->value ) && isset( $this->value['date'] ) ) {

Choose a reason for hiding this comment

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

Does this account for repeating datetime fields?

I'm seeing this issue mentioned in #330 as well. I was looking through the code and believe I have narrowed it down to the deleted line 215 failing to account for repeatable fields. However I havnt had the time to try to fix it.

Copy link
Member

Choose a reason for hiding this comment

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

I'll look into the repeatable field issues (hopefully tomorrow).

@jtsternberg jtsternberg merged commit f6be281 into CMB2:yivi-date-time-picker-fixes Nov 23, 2015
@jtsternberg
Copy link
Member

Thanks for all your hard work here. I've merged it in and have made some changes to reign the scope in a bit. It seems to work well in my testing, but please test on your end to verify.

users wont be able to use escape characters in their format strings. (E.g.: 'd \d\e m \d\e Y', or any other combinations).

Either that should be reflected in the docs, or that particular stripslashes should go... :P

What is the use-case where that is necessary?

But I guess we should also put there the acceptable characters for the format string (d, j, z, m, n, y, and Y, instead of the whole date() range).

Good idea, I updated the trunk branch of the wiki and will merge it to master when the next version of CMB2 is released with this update/change.
`

As an aside... for date/time combo (no timezone) we are saving an array with 'date' and 'time' as keys, while just saving a timestamp should be sufficient (and maybe cleaner?)

I saw that in your branch, but that is not part of how CMB2 currently works. I've removed it and it's using the timestamp as it should.

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