Add adventure custom post type #33

DavidCramer · 2018-04-06T07:52:03Z

Adds the adventure custom post type.

Fixes #7

DavidCramer · 2018-04-06T08:02:03Z

Still requires the additional metafields setup to capture the extra data, but otherwise as a basis for the custom post type definition, this is ready for review.

cc @ThierryA @westonruter

westonruter

For the meta fields, we'll want to use either Gutenberg blocks that use postmeta as their attribute sources.

westonruter · 2018-04-07T18:40:23Z

includes/class-amp-travel-cpt.php

+			'show_ui'             => true,
+			'show_in_nav_menus'   => true,
+			'show_in_menu'        => true,
+			'show_in_admin_bar'   => true,


This is missing show_in_rest which is required for Gutenberg.

westonruter · 2018-04-07T18:41:53Z

includes/class-amp-travel-cpt.php

+		 */
+		add_theme_support( 'post-thumbnails' );
+
+		// Image Sizes.


This should be a phpdoc comment to describe the filter.

DavidCramer · 2018-04-09T05:57:29Z

Thanks @westonruter - I have addressed the changes you requested. Feel free to review when you have time.

miina · 2018-04-09T11:54:04Z

@DavidCramer On the adventure post type -- did you also try editing in Gutenberg? When I create a new Adventure, publish it, and then change a setting (for example the content) and try to save again, I’m getting some errors + the title disappears from the view. Looks similar to behavior that got fixed here, however that was supposedly happening only if the CPT doesn't support title.

Wondering if that's something that only I'm experiencing or if it's an issue with Gutenberg.

If you have a moment could you try if it works OK for you?

miina · 2018-04-09T12:53:07Z

includes/class-amp-travel-cpt.php

+ *
+ * @package WPAMPTheme
+ */
+class AMP_Travel_CTP {


Looks like a typo, should probably be _CPT instead of _CTP.

oops yes, will fix

DavidCramer · 2018-04-09T13:00:11Z

@miina I haven't tested yet in Gutenberg yet as this was just the definition of the type. But I think I should put it back to WIP and get that aspect sorted.

DavidCramer · 2018-04-09T15:07:29Z

@westonruter - I initially did this just as the CPT definition. However, as @miina pointed out, it's not working yet with Gutenberg. I'll do more tests and clean it up first, then will let you know when you can review it again.

DavidCramer · 2018-04-11T07:20:25Z

@westonruter @ThierryA this is ready for review now. @miina I have tested with Gutenberg and it behaves as expected.

Use AMP_Travel_Theme::instantiate_classes(). And change AMP_Travel_CPT::__contstruct to init(). This uses the bootstrap mechanism in the develop branch.

kienstra

Almost Ready, A Few Points

Hi @DavidCramer,
Thanks for registering the new 'adventure' post type. There are a few points here, one of which might not need any action.

kienstra · 2018-04-11T17:20:37Z

includes/class-amp-travel-cpt.php

+			'description'           => __( 'Adventure Custom Post Type for travel theme.', 'travel' ),
+			'public'                => true,
+			'exclude_from_search'   => true,
+			'publicly_queryable'    => true,


The following arguments have the default value, and are not needed:

publicly_queryable

show_ui

show_in_nav_menus

show_in_menu

show_in_admin_bar

capability_type

hierarchical

query_var

can_export

rest_controller_class

@kienstra - yup, I'll remove them.

kienstra · 2018-04-11T17:26:35Z

includes/class-amp-travel-cpt.php

+			'280x158',
+			'240x135',
+			'160x90',
+			'122x67',


Are all of these image sizes needed?

You know more about the specifics than I do. But it seems excessive to create 15 image sizes for every image uploaded.

@kienstra - no idea. but it's a good point. I took these from the ampconf theme as a starting point. I'll reduce it down to what I think should be needed for the current blocks, etc.

This is something that we should revisit. But I'm going to merge this PR to unblock other PRs.

These look to have been from PR #33. And they might be valid. But defer the discussion of those to that PR.

We might need custom image sizes, so continue this conversation. But several PRs depend on this PR, so it would help to merge it.

In that class, make all functions public. Change __construct() to init(), and move the requiring of class-amp-travel-cpt.php to AMP_Travel_Theme::includes(). Thanks to David for creating that file. This keeps all of the logic of the plugin files self-sufficient. functions.php wil only have to require class-amp-travel-theme.php. And call amp_travel_theme()->init().

And its instantiation. That class isn't in this branch.

Use the new includes() method to require it. And instantiate and init() it in: instantiate_classes().

kienstra

Approved

If it's alright, I applied the 2 review points I made with this commit.

@DavidCramer, could you please look at whether there are any more image sizes needed here? I deleted the add_image_size() call for now, to merge this.

DavidCramer · 2018-04-16T09:41:04Z

Thanks @kienstra. Can always add additional sizes if needed later. It's all good.

DavidCramer added 2 commits April 6, 2018 09:41

Add adventure custom post type

a3e5a9f

Refactor for easy extraction later

c4bed08

DavidCramer requested review from ThierryA and westonruter April 6, 2018 08:00

DavidCramer changed the title ~~[WIP] Add adventure custom post type~~ Add adventure custom post type Apr 6, 2018

westonruter reviewed Apr 7, 2018

View reviewed changes

Set show_in_rest and add phpdoc to filter

67e54bf

Add plural slug for rest base.

f77bf24

miina added a commit that referenced this pull request Apr 9, 2018

Merge #33.

93240b3

miina reviewed Apr 9, 2018

View reviewed changes

DavidCramer changed the title ~~Add adventure custom post type~~ [WIP] Add adventure custom post type Apr 9, 2018

kienstra mentioned this pull request Apr 10, 2018

Add Activity and Location taxonomies. #31

Merged

rename typo in class name

a77c7df

DavidCramer changed the title ~~[WIP] Add adventure custom post type~~ Add adventure custom post type Apr 11, 2018

DavidCramer mentioned this pull request Apr 11, 2018

[WIP] Implement PSTT theme #38

Closed

4 tasks

ThierryA requested a review from kienstra April 11, 2018 11:52

Merge in develop, resolve conflicts, use theme bootstrap.

5fd707e

Use AMP_Travel_Theme::instantiate_classes(). And change AMP_Travel_CPT::__contstruct to init(). This uses the bootstrap mechanism in the develop branch.

kienstra self-assigned this Apr 11, 2018

kienstra suggested changes Apr 11, 2018

View reviewed changes

kienstra assigned DavidCramer and unassigned kienstra Apr 11, 2018

kienstra mentioned this pull request Apr 12, 2018

[Gutenberg] Add Popular homepage block. #32

Merged

4 tasks

kienstra added a commit that referenced this pull request Apr 12, 2018

Revert changes to style.css.

f0c1cfc

These look to have been from PR #33. And they might be valid. But defer the discussion of those to that PR.

Remove add_image_size(), default arguments for register_post_type().

5ee6fdc

We might need custom image sizes, so continue this conversation. But several PRs depend on this PR, so it would help to merge it.

kienstra added 3 commits April 12, 2018 16:18

Remove require_once of class-amp-travel-taxonomies.php.

5f4ab21

And its instantiation. That class isn't in this branch.

Merge in develop, resolve conflicts, require taxonomies.

efbc8ae

Use the new includes() method to require it. And instantiate and init() it in: instantiate_classes().

kienstra approved these changes Apr 12, 2018

View reviewed changes

kienstra merged commit c420080 into develop Apr 12, 2018

kienstra deleted the feature/7-declare-cpt branch April 12, 2018 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add adventure custom post type #33

Add adventure custom post type #33

DavidCramer commented Apr 6, 2018

DavidCramer commented Apr 6, 2018

westonruter left a comment

westonruter Apr 7, 2018

westonruter Apr 7, 2018

DavidCramer commented Apr 9, 2018

miina commented Apr 9, 2018

miina Apr 9, 2018

DavidCramer Apr 9, 2018

DavidCramer commented Apr 9, 2018

DavidCramer commented Apr 9, 2018

DavidCramer commented Apr 11, 2018

kienstra left a comment

kienstra Apr 11, 2018 •

edited

Loading

DavidCramer Apr 12, 2018

kienstra Apr 11, 2018 •

edited

Loading

DavidCramer Apr 12, 2018

kienstra Apr 12, 2018

kienstra left a comment

DavidCramer commented Apr 16, 2018

Add adventure custom post type #33

Add adventure custom post type #33

Conversation

DavidCramer commented Apr 6, 2018

DavidCramer commented Apr 6, 2018

westonruter left a comment

Choose a reason for hiding this comment

westonruter Apr 7, 2018

Choose a reason for hiding this comment

westonruter Apr 7, 2018

Choose a reason for hiding this comment

DavidCramer commented Apr 9, 2018

miina commented Apr 9, 2018

miina Apr 9, 2018

Choose a reason for hiding this comment

DavidCramer Apr 9, 2018

Choose a reason for hiding this comment

DavidCramer commented Apr 9, 2018

DavidCramer commented Apr 9, 2018

DavidCramer commented Apr 11, 2018

kienstra left a comment

Choose a reason for hiding this comment

kienstra Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

DavidCramer Apr 12, 2018

Choose a reason for hiding this comment

kienstra Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

DavidCramer Apr 12, 2018

Choose a reason for hiding this comment

kienstra Apr 12, 2018

Choose a reason for hiding this comment

kienstra left a comment

Choose a reason for hiding this comment

DavidCramer commented Apr 16, 2018

kienstra Apr 11, 2018 •

edited

Loading

kienstra Apr 11, 2018 •

edited

Loading