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

Add endtime field to wpt_events #136

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
@jbrandligt
Contributor

jbrandligt commented Jun 25, 2015

No description provided.

@slimndap

This comment has been minimized.

Show comment
Hide comment
@slimndap

slimndap Jun 25, 2015

Thank you!

However, it looks like you copied the entire time() method, which ws designed to handle both start and end times.

It would be better to add two new methods: endtime() and enddate(). Both methods would just call their generic equivalents, but with the right $args:

$endtime = $this->time(
  array( 
    'start' => false,
    'html' => $html,
    'filters' => $filters,
  )
);

Thank you!

However, it looks like you copied the entire time() method, which ws designed to handle both start and end times.

It would be better to add two new methods: endtime() and enddate(). Both methods would just call their generic equivalents, but with the right $args:

$endtime = $this->time(
  array( 
    'start' => false,
    'html' => $html,
    'filters' => $filters,
  )
);

This comment has been minimized.

Show comment
Hide comment
@jbrandligt

jbrandligt Jun 25, 2015

Owner

Indeed that is what I did. I think you are right, I'll see if I can solve it that way. Thanks.

Owner

jbrandligt replied Jun 25, 2015

Indeed that is what I did. I think you are right, I'll see if I can solve it that way. Thanks.

This comment has been minimized.

Show comment
Hide comment
@slimndap

slimndap Jun 25, 2015

And perhaps we add two more methods:

starttime() and startdate().

And to make it event more user friendly, we could turn it into 8 new methods:

starttime()
starttime_html()
startdate()
startdate_html()
endtime()
endtime_html()
enddate()
enddate_html()

Where all '_html' method return the HTML equivalent of the corresponding field.

Jeroen

And perhaps we add two more methods:

starttime() and startdate().

And to make it event more user friendly, we could turn it into 8 new methods:

starttime()
starttime_html()
startdate()
startdate_html()
endtime()
endtime_html()
enddate()
enddate_html()

Where all '_html' method return the HTML equivalent of the corresponding field.

Jeroen

This comment has been minimized.

Show comment
Hide comment
@jbrandligt

jbrandligt Jun 25, 2015

Owner

We will probably have to tweak the original time and date methods a little to make sure it generates the right html output when called from another method.
For instance this line: $html = '<div class="'.self::post_type_name.'_time">';

Owner

jbrandligt replied Jun 25, 2015

We will probably have to tweak the original time and date methods a little to make sure it generates the right html output when called from another method.
For instance this line: $html = '<div class="'.self::post_type_name.'_time">';

This comment has been minimized.

Show comment
Hide comment
@slimndap

slimndap Jun 25, 2015

Maybe:

$html = '<div class="'.self::post_type_name.'_time '.self::post_type_name.'_endtime">';

Maybe:

$html = '<div class="'.self::post_type_name.'_time '.self::post_type_name.'_endtime">';

This comment has been minimized.

Show comment
Hide comment
@slimndap

slimndap Jun 25, 2015

We can also do it the other way around. Make the date() and time() methods much simpler. All they do is call one of the 8 new methods, based on $args.

We can also do it the other way around. Make the date() and time() methods much simpler. All they do is call one of the 8 new methods, based on $args.

This comment has been minimized.

Show comment
Hide comment
@jbrandligt

jbrandligt Jun 25, 2015

Owner

Yes, I think that might be a better approach, more logical that way. I'll see what I can come up with.

Owner

jbrandligt replied Jun 25, 2015

Yes, I think that might be a better approach, more logical that way. I'll see what I can come up with.

This comment has been minimized.

Show comment
Hide comment
@slimndap

slimndap Jun 25, 2015

@jbrandligt

This comment has been minimized.

Show comment
Hide comment
@jbrandligt

jbrandligt Jun 25, 2015

Owner

please ignore commit...

Owner

jbrandligt commented on 8df8a2d Jun 25, 2015

please ignore commit...

@slimndap slimndap added this to the v0.12 milestone Jun 25, 2015

Partial commit, starttime and endtime methods implemented, both text …
…and html. startdate and enddate still to come.
@jbrandligt

This comment has been minimized.

Show comment
Hide comment
@jbrandligt

jbrandligt Jun 26, 2015

Contributor

Hey, I did a partial commit of my work so far. Could you have a look and tell me if this is what you had in mind?

Contributor

jbrandligt commented Jun 26, 2015

Hey, I did a partial commit of my work so far. Could you have a look and tell me if this is what you had in mind?

@slimndap

This comment has been minimized.

Show comment
Hide comment
@slimndap

slimndap Jun 26, 2015

[$field] is no longer needed here.
$this->endtime_html is enough.

[$field] is no longer needed here.
$this->endtime_html is enough.

@slimndap

This comment has been minimized.

Show comment
Hide comment
@slimndap

slimndap Jun 26, 2015

You should replace $args['start'] with false.

You should replace $args['start'] with false.

@slimndap

This comment has been minimized.

Show comment
Hide comment
@slimndap

slimndap Jun 26, 2015

You can use $this->endtime() over here.

You can use $this->endtime() over here.

This comment has been minimized.

Show comment
Hide comment
@jbrandligt

jbrandligt Jun 26, 2015

Owner

This one gives me trouble: "Fatal error: Can't use method return value in write context"
It seems I have to specify a place where apply_filters can store its output. Can be [$anyfield]. If I omit [$field] entirely the result output is 1.

Owner

jbrandligt replied Jun 26, 2015

This one gives me trouble: "Fatal error: Can't use method return value in write context"
It seems I have to specify a place where apply_filters can store its output. Can be [$anyfield]. If I omit [$field] entirely the result output is 1.

This comment has been minimized.

Show comment
Hide comment
@slimndap

slimndap Jun 26, 2015

I think you can just leave this whole part out (lines 920 - 930).
And on line 941, you can replace $this->endtime_html[ $field ] with $this->endtime().

I think you can just leave this whole part out (lines 920 - 930).
And on line 941, you can replace $this->endtime_html[ $field ] with $this->endtime().

This comment has been minimized.

Show comment
Hide comment
@jbrandligt

jbrandligt Jun 26, 2015

Owner

Indeed! Works fine now.

Owner

jbrandligt replied Jun 26, 2015

Indeed! Works fine now.

@slimndap

This comment has been minimized.

Show comment
Hide comment
@slimndap

slimndap Jun 26, 2015

You can skip the WPT_Filters here. They are already applied in endtime().

You can skip the WPT_Filters here. They are already applied in endtime().

@slimndap

This comment has been minimized.

Show comment
Hide comment
@slimndap

slimndap Jun 26, 2015

Owner

And yes: this is what I had in mind.

Owner

slimndap commented Jun 26, 2015

And yes: this is what I had in mind.

@jbrandligt

This comment has been minimized.

Show comment
Hide comment
@jbrandligt

jbrandligt Jun 26, 2015

Contributor

Ok cool, I'll clean up the code and implement the date methods too then.

Contributor

jbrandligt commented Jun 26, 2015

Ok cool, I'll clean up the code and implement the date methods too then.

slimndap added a commit that referenced this pull request Jun 26, 2015

Added tests that confirm the lack of support for {{starttime}}, {{sta…
…rtdate}}, {{endtime}} and {{enddate}} placeholders.

See #60 and #136.
Startdate and enddate methods implemented too and some code cleanup. …
…Date and and time methods are in fact identical to startdate_html and starttime_html methods but left for backward compatibility.
@slimndap

This comment has been minimized.

Show comment
Hide comment
@slimndap

slimndap Jun 27, 2015

Owner

I just merged your code back into mine.

Owner

slimndap commented Jun 27, 2015

I just merged your code back into mine.

@jbrandligt

This comment has been minimized.

Show comment
Hide comment
@jbrandligt

jbrandligt Jun 27, 2015

Contributor

Sweet!

Contributor

jbrandligt commented Jun 27, 2015

Sweet!

@slimndap

This comment has been minimized.

Show comment
Hide comment

slimndap added a commit that referenced this pull request Jun 28, 2015

Merge branch 'issue-60' into v0.12
* issue-60:
  wpt_event.php is now PHP CodeSniffer approved.
  Improved the code of @jbrandligt: - Removed all PHP notices. - Fully deprecated WPT_Event::date() and WPT_Event::time(). - Complete the PHPDocs for the new methods.
  Startdate and enddate methods implemented too and some code cleanup. Date and and time methods are in fact identical to startdate_html and starttime_html methods but left for backward compatibility.
  Added tests that confirm the lack of support for {{starttime}}, {{startdate}}, {{endtime}} and {{enddate}} placeholders. See #60 and #136.
  Partial commit, starttime and endtime methods implemented, both text and html. startdate and enddate still to come.
  New approach on endtime method, work in progress...
  Add endtime field to wpt_events

Conflicts:
	functions/wpt_event.php

@slimndap slimndap closed this Jul 3, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment