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

Jump to bottom

Add new utility function #1739

Merged
merged 8 commits into from Jan 4, 2018
Merged

Add new utility function #1739

merged 8 commits into from Jan 4, 2018

Conversation

mitogh
Copy link
Contributor

@mitogh mitogh commented Dec 28, 2017 •
edited

Add tribe_is_home_events function similar to is_home in WP to test if we are on the homepage of the events.

See Ticket: https://central.tri.be/issues/42195

@mitogh
Add utilify function tribe_is_home_events
d2384ee 
Add function to see if we are on the home of events similar to is_home
in WordPress.

Ticket: https://central.tri.be/issues/42195
@mitogh
Update return name for consistency
f8d7581 
As on the file majority of instances uses 'bool' in the return statment
it might be a good idea to be consistent.
@mitogh mitogh added code review Status: requires a code review. and removed code review Status: requires a code review. labels Dec 28, 2017
bordoni
bordoni previously requested changes Dec 30, 2017
View reviewed changes
src/functions/template-tags/general.php Outdated
* Utility function to test if we are on the home of events, it makes test the case when the page is set to be on
* the frontpage of the site and if the user is on that page is on the homepage or if the user is on the events page
* where the eventDisplay is set to default.
*
Copy link
Member

Choose a reason for hiding this comment

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

add @since

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, changes has been updated.

@bordoni bordoni added the enhance Status: Code could use some enhancements before merging. label Dec 30, 2017
@mitogh mitogh removed the enhance Status: Code could use some enhancements before merging. label Jan 2, 2018
@mitogh
Add new Utility tribe_is_events_front_page method
99661ee 
This new Utility function allow to test if we are on the front page of
the events page as sometimes tribe_is_events_home is not enought and
and both can be used in different locations such as #1748.
@mitogh
Copy link
Contributor Author

mitogh commented Jan 3, 2018

I added a new method here @bordoni is basically a separation of the same function in two as I'm going to need that one on #1748, let me know if you have any questions.

@bordoni bordoni added merge Status: ready to merge. and removed code review Status: requires a code review. labels Jan 3, 2018
barryhughes
barryhughes reviewed Jan 3, 2018
View reviewed changes
src/functions/template-tags/general.php Outdated

/**
* Function to test if we are on the front page of the events, as WordPress language front page is different from
* is_home, if we have a front page on the reading options and if the uzer is on that page, this function will
Copy link
Contributor

Choose a reason for hiding this comment

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

* User

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

barryhughes
barryhughes reviewed Jan 3, 2018
View reviewed changes
src/functions/template-tags/general.php Outdated
@@ -1669,4 +1669,60 @@ function tribe_separated_field( $body, $separator, $field ) {

return $field ? $body_and_separator . $field : $body;
}

/**
* Function to test if we are on the front page of the events, as WordPress language front page is different from
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good - but sometimes the information is easier to digest if we start with a nice and short sentence explaining very briefly what the function does - with any further explanations, notes or examples into additional paragraphs. For instance:

/**
 * Tests if we are on the site homepage and if it is set to display the main events page.
 *
 * <notes about the differences with WordPress's definition of front page, examples of
 * real usage etc>
 * 
 * @since TBD
 *
 * @return bool
 */

Note also the line break between 'meta' information such as @since and the @return tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up.

@barryhughes
Copy link
Contributor

@mitogh - I like these functions but there's a possible bear-trap we ought to warn users of: if code within an events template calls tribe_is_events_front_page(), for example, then it will work so long as the front page was loaded directly. If the user pages forward by ajax, though, it will cease to work. This could cause some confusion and is probably worth warning about.

We should also add a changelog entry noting the addition of these new template tags.

@barryhughes barryhughes added code review Status: requires a code review. enhance Status: Code could use some enhancements before merging. needs changelog Needs a changelog entry before merging. and removed merge Status: ready to merge. labels Jan 3, 2018
@mitogh
Copy link
Contributor Author

mitogh commented Jan 3, 2018

Thanks for the feedback @barryhughes. However I'm not really sure about the changelog. just a couple of questions there:

  1. Should we edit the readme.txt file?
  2. What version and date should we add?

Thanks.

@mitogh mitogh added the question Needs an answer to one or more questions before merging. label Jan 3, 2018
@barryhughes
Copy link
Contributor

Should we edit the readme.txt file?

Yes 👍

What version and date should we add?

The date should generally be TBD at this stage, but the new version number can be found at the top of the readme file (though, if in doubt, TBD is fine for this, too).

In this case, you can add your entry within this block.

@mitogh mitogh removed enhance Status: Code could use some enhancements before merging. needs changelog Needs a changelog entry before merging. question Needs an answer to one or more questions before merging. labels Jan 3, 2018
@mitogh mitogh added code review Status: requires a code review. and removed code review Status: requires a code review. labels Jan 3, 2018
@mitogh
Copy link
Contributor Author

mitogh commented Jan 3, 2018

Changes has been added diff is not able to pick the changes, on the missing changes.

cc @barryhughes @bordoni

@barryhughes barryhughes added the merge Status: ready to merge. label Jan 3, 2018
@barryhughes
Copy link
Contributor

👍

@mitogh mitogh dismissed bordoni’s stale review January 4, 2018 00:11

Diff is not correct changes has been introduced correctly.

@mitogh mitogh merged commit eb8f2aa into release/M18.01 Jan 4, 2018
@mitogh mitogh deleted the fix/42195-add-util-method branch January 4, 2018 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barryhughes barryhughes barryhughes approved these changes

@bordoni bordoni bordoni left review comments

Assignees
No one assigned
Labels
code review Status: requires a code review. merge Status: ready to merge.
Projects
None yet
Milestone
No milestone
3 participants
@mitogh @barryhughes @bordoni