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

datepicker and timepicker fixes #318

Closed
wants to merge 23 commits into from
Closed

datepicker and timepicker fixes #318

wants to merge 23 commits into from

Conversation

yivi
Copy link
Contributor

@yivi yivi commented May 8, 2015

Support for date-format and time-format in text_date and text_time fields. They accept a php formatted string, which is converted to a js formatted string and injected in the data attribute so the pickers can be initialized appropriately. Fixes #282, fixes #300

yivi added 20 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
@jtsternberg
Copy link
Member

Thanks a ton for putting time in to work on this issue. To help me better review your PR, I have a few requests:

  • Remove all edits/changes that are not related to this bug-fix/issue
  • Remove all white-space changes (same as above). This includes doc-block edits.
  • Try to keep your PR as focused on solving a single problem as possible

I like the edits you made (better doc-blocks, etc), but these things should be separate PRs. It's much easier to merge a PR that is simply doc-block edits than one that is changing functionality, or worse, both.

Can you submit separate PRs to split these concerns and make it easier to review/merge? Thank you.

@yivi
Copy link
Contributor Author

yivi commented Jul 11, 2015

Hi there. Will do! 

Sorry about those side tracked changes. Code style happened because I had my ide to auto format to wp guidelines on save and didn't realize, and the docblocks was probably me going over the code inspection notices and warnings before committing. 

I'll make this changes on Monday eve. Regards and have a nice weekend! 


Sent from Mailbox

On Thu, Jul 9, 2015 at 9:07 PM, Justin Sternberg notifications@github.com
wrote:

Thanks a ton for putting time in to work on this issue. To help me better review your PR, I have a few requests:

  • Remove all edits/changes that are not related to this bug-fix/issue
  • Remove all white-space changes (same as above). This includes doc-block edits.
  • Try to keep your PR as focused on solving a single problem as possible
    I like the edits you made (better doc-blocks, etc), but these things should be separate PRs. It's much easier to merge a PR that is simply doc-block edits than one that is changing functionality, or worse, both.
    Can you submit separate PRs to split these concerns and make it easier to review/merge? Thank you.

    Reply to this email directly or view it on GitHub:
    datepicker and timepicker fixes #318 (comment)

@jtsternberg
Copy link
Member

Ok thanks again! I really do like the formatting cleanup, so feel free to submit another separate PR if you're up for it.

@yivi
Copy link
Contributor Author

yivi commented Jul 13, 2015

I think I removed all formatting changes from the PR.
Let me know if you see something you think it would benefit from a bit more work.

Regards!

@yivi yivi mentioned this pull request Jul 13, 2015
@jtsternberg
Copy link
Member

Thank you for cleaning it up, that helps a ton. Now that I can see what's going on, I see you are using DateTime::createFromFormat which unfortunately is a PHP 5.3 feature. We have to support the same versions of PHP that WordPress does, which is 5.2.4. Maybe you can check if that feature exists and use it and if not, create a fallback solution?

@yivi
Copy link
Contributor Author

yivi commented Jul 14, 2015

5.2... I hope WP Core gets up to speed at some point; 5.2 has been eol for more than 4 years now... and 7 is around the corner. :P

Pushed a new version that checks if create_date_from_format exists, and defines it if not.

Thanks and regards!

@jtsternberg
Copy link
Member

@yivi Why didn't you update the text_datetime_timestamp field type w/ your changes? I'm concerned that the changes you've made are not taking all the field types and/or scenarios into account.

@jtsternberg
Copy link
Member

Nice work so far. I've merged your branch into trunk and created a new branch. let's collaborate on this branch instead of trunk until it's ready to go: https://github.com/WebDevStudios/CMB2/tree/yivi-date-time-picker-fixes

@yivi
Copy link
Contributor Author

yivi commented Aug 15, 2015

Sorry for the late reply. I was out on holidays on a cycling trip, very
much away from the keyboard.

I'll resume work this week. I'll take a look a at what's left, hopefully we
can have everything wrapped up soon enough.

Regards!

2015-07-29 18:44 GMT+02:00 Justin Sternberg notifications@github.com:

Nice work so far. I've merged your branch into trunk and created a new
branch. let's collaborate on this branch instead of trunk until it's ready
to go:
https://github.com/WebDevStudios/CMB2/tree/yivi-date-time-picker-fixes


Reply to this email directly or view it on GitHub
#318 (comment).

@jtsternberg
Copy link
Member

This is almost good to go. Just need to make sure that all the date type fields are accounted for in this change.

@jtsternberg
Copy link
Member

@yivi will you be able to add support for ALL the date type fields?

@tw2113
Copy link
Contributor

tw2113 commented Oct 7, 2015

Is there any way we could add in fetching the date_format option from the user's General Settings as a default format value as well?

@jtsternberg
Copy link
Member

Also, how does this differ from #446?

@yivi
Copy link
Contributor Author

yivi commented Oct 8, 2015

Justin,

I've tried my fixes with all these date/time fields, and they are working:

Regards

2015-10-07 17:57 GMT+02:00 Justin Sternberg notifications@github.com:

Also, how does this differ from #446
#446?


Reply to this email directly or view it on GitHub
#318 (comment).

@yivi
Copy link
Contributor Author

yivi commented Oct 8, 2015

Just merged current trunk and tested it on a couple of working sites.

Things look good.

2015-10-08 20:39 GMT+02:00 Yivi nardus@gmail.com:

Justin,

I've tried my fixes with all these date/time fields, and they are working:

Regards

2015-10-07 17:57 GMT+02:00 Justin Sternberg notifications@github.com:

Also, how does this differ from #446
#446?


Reply to this email directly or view it on GitHub
#318 (comment).

@yivi
Copy link
Contributor Author

yivi commented Oct 21, 2015

Work continued in #446

@yivi yivi closed this Oct 21, 2015
@yivi yivi deleted the trunk branch October 22, 2015 15:55
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