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

Conversation

@yivi
Copy link
Contributor

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
yivi
Outputs data-datepicker into date_text, so each datepicker can be ini…
…tialized with its own date-format

The js initializer reads data-datepicker to set each datepicker properly
yivi
Merge remote-tracking branch 'upstream/trunk' into trunk
Conflicts:
	includes/CMB2_Sanitize.php
	includes/CMB2_Types.php
	js/cmb2.min.js
	languages/cmb2-es_ES.mo
	languages/cmb2-es_ES.po
yivi
yivi
Merge remote-tracking branch 'upstream/trunk' into trunk
Conflicts:
	js/cmb2.min.js
	languages/cmb2-es_ES.mo
	languages/cmb2-es_ES.po
@jtsternberg

This comment has been minimized.

Copy link
Member

commented Jul 9, 2015

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

This comment has been minimized.

Copy link
Contributor Author

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:
    #318 (comment)
@jtsternberg

This comment has been minimized.

Copy link
Member

commented Jul 11, 2015

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

This comment has been minimized.

Copy link
Contributor Author

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 pushed a commit to yivi/CMB2 that referenced this pull request Jul 13, 2015
@yivi yivi referenced this pull request Jul 13, 2015
@jtsternberg

This comment has been minimized.

Copy link
Member

commented Jul 14, 2015

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Jul 29, 2015

@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

This comment has been minimized.

Copy link
Member

commented Jul 29, 2015

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Sep 3, 2015

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

@jtsternberg

This comment has been minimized.

Copy link
Member

commented Oct 7, 2015

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

@tw2113

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Oct 7, 2015

Also, how does this differ from #446?

@yivi

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2015

Work continued in #446

@yivi yivi closed this Oct 21, 2015

@yivi yivi deleted the yivi:trunk branch Oct 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.