Skip to content
This repository has been archived by the owner on Jan 10, 2023. It is now read-only.

Add Activity and Location taxonomies. #31

Merged
merged 4 commits into from
Apr 11, 2018

Conversation

miina
Copy link
Contributor

@miina miina commented Apr 4, 2018

Fixes #27.

Adds custom taxonomies activity and location for both post and adventure post types.

@miina
Copy link
Contributor Author

miina commented Apr 4, 2018

Considering that the additional term meta required by blocks will be added within the block tickets, is there anything else that could be necessary for the custom taxonomy within the theme? Was thinking about custom capabilities but not sure if these would be needed within the theme context. Thoughts? (cc @ThierryA @westonruter )

Otherwise the PR should be ready for review.

@westonruter
Copy link
Contributor

Additional term meat can be added in subsequent PR as well as custom caps, if needed.

@ThierryA
Copy link
Contributor

ThierryA commented Apr 5, 2018

Thanks @miina, since taxonomy and post type fall under "functionalities", I would love to see the code extracted from the "functions.php" into directory with structured files and classes which will be the hours of upcoming functionalities and API functions such as the upcoming query functions etc.

Thus also to make it very easy be separated as a plugin later on.

@miina
Copy link
Contributor Author

miina commented Apr 5, 2018

@ThierryA: Created includes folder and added a separate taxonomy-related class there within e39a30d. Let me know if that's okay like this or if there's anything else that should be improved.

Copy link
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

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

Minor Changes That Depend On #13

Hi @miina,
This looks good, and it's almost ready. This will depend on #13 being merged, but could you please address the points below when it is?

Thanks!

Update: per @westonruter's comments below, this doesn't depend on #13, as the taxonomy logic doesn't need to be OOP.

* AMP_Travel_Taxonomies constructor.
*/
public function __construct() {
add_action( 'init', array( $this, 'register_taxonomies' ) );
Copy link
Contributor

@kienstra kienstra Apr 10, 2018

Choose a reason for hiding this comment

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

@miina, could you please create an init() method, and move this add_action()call inside it?

Update: not needed, per @westonruter's comments below.

}
}

new AMP_Travel_Taxonomies();
Copy link
Contributor

@kienstra kienstra Apr 10, 2018

Choose a reason for hiding this comment

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

This will depend on the work in PR #13 (comment). But it'd be great to have the instantiation of AMP_Travel_Taxonomies in a method like AMP_Travel_Theme::instantiate_classes(), or some method that AMP_Travel_Theme::init() calls.

Copy link
Contributor

@westonruter westonruter Apr 10, 2018

Choose a reason for hiding this comment

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

Note that in this case it would instead then be:

$amp_travel_taxonomies = new AMP_Travel_Taxonomies();
$amp_travel_taxonomies->init();

Unless the class switches to using static methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note also that new AMP_Travel_Taxonomies() shouldn't be in this file. The PHP file should just contain the one class. So functions.php should require the file and then do the above.

All of this to say, we may be complicating things 😄 Feel free to just add a new global function called amp_travel_register_taxonomies() and rename this file to taxonomies.php and include the add_action( 'init', 'amp_travel_register_taxonomies' ); in the file. Since this is a theme you can feel free to have more “functional programming” instead of OOP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the comments, @kienstra and @westonruter.

Based on @ThierryA's comment and @kienstra's PR review on the blocks base layout it seemed like using classes would be the preferred way. Happy to change it back to using global functions, however, maybe it would make sense to use the same structure throughout the code? For example if taxonomy-related functionality is not in OOP, should Gutenberg block-related logic still be in OOP? Or should we either use one or the other?

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@miina I merged this into the PSTT theme which should give us a few structures. See https://github.com/xwp/travel/pull/38/files#diff-8ab8775c3cc79aadce48cad633a7fde3R109
I will be looking at pulling in the structures for the blocks as well.

Copy link
Contributor

@kienstra kienstra Apr 11, 2018

Choose a reason for hiding this comment

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

Hi @miina,
It definitely makes sense to have global functions in a theme, instead of OOP. But my preference would be to use OOP for the taxonomy and block classes, in case we need to move this into a plugin later. Still, I may not have understood exactly what we might have to move into a plugin.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm merging this now to unblock work, but this can remain an open conversation about whether to use OOP or global functions.

* Register 'activity' and 'location' taxonomy.
*/
public function register_taxonomies() {
register_taxonomy( 'activity', array( 'adventure', 'post' ), array(
Copy link
Contributor

@kienstra kienstra Apr 10, 2018

Choose a reason for hiding this comment

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

There's no action needed now. But when PR #33 is merged, this 'adventure' string should probably be AMP_Travel_CPT::POST_TYPE_SLUG_SINGLE.

@DavidCramer DavidCramer mentioned this pull request Apr 11, 2018
4 tasks
@miina
Copy link
Contributor Author

miina commented Apr 11, 2018

Hi @kienstra:

Thanks again for reviewing.
The newly updated develop is merged and also addressed the OOP-related changes since using classes was something that @ThierryA specifically seemed to ask. Let me know if any additional changes or undoing the OOP are needed. Thanks!

Copy link
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

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

Approved

Hi @miina,
Thanks for your great work here. This is approved.

@kienstra kienstra merged commit c361a86 into develop Apr 11, 2018
@kienstra kienstra deleted the feature/27-add_custom_taxonomies branch April 11, 2018 18:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants