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

Add WordPress Area CPT #15014

Merged
merged 1 commit into from May 8, 2019

Conversation

Projects
None yet
3 participants
@jorgefilipecosta
Copy link
Member

commented Apr 17, 2019

Description

This PR is straight to the point and just proposes an implementation for the wp_area CPT required to implement the widgets RFC described in https://github.com/WordPress/gutenberg/blob/add/blocks-in-widget-areas-rfc/docs/rfcs/blocks-in-widget-areas.md

How has this been tested?

I verified the CPT works as expected by going to /edit.php?post_type=wp_area (this probably should be disabled but it is useful for testing).
I verified I can use the rest API with this CPT e.g:

wp.apiFetch({
    path: '/wp/v2/block-areas',
    method: 'GET',
}).then(console.log);
'wp_area',
array(
'labels' => array(
'name' => _x( 'WordPress Area', 'post type general name' ),

This comment has been minimized.

Copy link
@youknowriad

youknowriad Apr 17, 2019

Contributor

Block Area is probably a better name like suggested by @mapk

'public' => false,
'show_ui' => true,
'show_in_menu' => false,
'show_in_rest' => true,

This comment has been minimized.

Copy link
@youknowriad

youknowriad Apr 17, 2019

Contributor

Are we following-up with the REST endpoints PR? we might want to disable the built-in ones if it's the case?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Apr 17, 2019

Author Member

My plan was to leave the built-in ones. The REST endpoints that we are following up are specific to widget areas, and they receive as the identifier a widget area name. What if I want to change wp_area by its id (numeric)? What if I want to do other actions (list all areas widget/navigations..., search areas or use other nice features from the posts endpoint)?
It seemed to allow direct access to the wp_areas endpoint did have big cons and had some advantages.
The new endpoint may end up referencing the same thing but it references using a different identifier, and the new endpoint is widget specific (returning widget specific information besides the post itself being referenced)

'item_updated' => __( 'WordPress area updated.' ),
),
'public' => false,
'show_ui' => true,

This comment has been minimized.

Copy link
@youknowriad

youknowriad Apr 17, 2019

Contributor

This should probably be false?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Apr 17, 2019

Author Member

Yes, show_ui should be false, it is true to make testing easier.

@@ -126,3 +126,58 @@ function gutenberg_legacy_widget_settings( $settings ) {
return $settings;
}
add_filter( 'block_editor_settings', 'gutenberg_legacy_widget_settings' );
function create_wp_area_post_type() {

This comment has been minimized.

Copy link
@aduth

aduth Apr 29, 2019

Member

Should it be prefixed with gutenberg_ ? I'd suggest it, unless the plan is for this function to be ported upstream directly, which I don't think is how it would be adopted.

Prior art: https://github.com/WordPress/wordpress-develop/blob/23867a2d7748d911d36107d39a6ea36dc834f763/src/wp-includes/post.php#L13-L432

Show resolved Hide resolved lib/widgets.php
@@ -126,3 +126,58 @@ function gutenberg_legacy_widget_settings( $settings ) {
return $settings;
}
add_filter( 'block_editor_settings', 'gutenberg_legacy_widget_settings' );
function create_wp_area_post_type() {

This comment has been minimized.

Copy link
@aduth

aduth Apr 29, 2019

Member

Missing PHPDoc?

@jorgefilipecosta jorgefilipecosta force-pushed the add/wordpress-area-cpt branch from a64d42a to 1868ca2 Apr 30, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Hi @aduth, thank you for the review, your comments were addressed.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Do you think we can start landing some of these widgets related PRs to move forward?

@youknowriad youknowriad requested a review from aduth May 7, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

Do you think we can start landing some of these widgets related PRs to move forward?

Hi @youknowriad, yes I think widget related PR's should be merged allowing us to have a prototype and try different approaches if we find problems.

'public' => false,
'show_ui' => false,
'show_in_menu' => false,
'show_in_rest' => true,

This comment has been minimized.

Copy link
@aduth

aduth May 7, 2019

Member

Should we assign a rest_base ? The post type wp_block was defined with rest_base of 'blocks' (source), following the general pattern of exposing on the plural form of the base name "block".

Suggested change
'show_in_rest' => true,
'show_in_rest' => true,
'rest_base' => 'areas',

Alternatively, the original RFC proposed the base 'widget-areas':

https://github.com/WordPress/gutenberg/blob/add/blocks-in-widget-areas-rfc/docs/rfcs/blocks-in-widget-areas.md#rest-api

Perhaps 'block-areas' if that's the desired naming, per prior review comments in this pull request.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta May 7, 2019

Author Member

Good point, I updated the rest_base to 'block-areas'.

This comment has been minimized.

Copy link
@aduth

aduth May 8, 2019

Member

I'd also wonder if it's worth considering to include in an experimental namespace, like at #15015

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta May 8, 2019

Author Member

In CPT's adding a namespace does not work. I used __experimental/block-areas as the rest base which works well but is a little bit "hacky". I can revert this change if people prefer the previous version without "__experimental/".

'capabilities' => array(
// You need to be able to edit posts, in order to read blocks in their raw form.
'read' => 'edit_posts',
// You need to be able to publish posts, in order to create blocks.

This comment has been minimized.

Copy link
@aduth

aduth May 7, 2019

Member

These comments seem to be copy-paste from the registration of 'wp_block', but aren't representative of what the actual values are (i.e. you need to be able to edit theme options, not publish posts).

https://github.com/WordPress/wordpress-develop/blob/96649a5ad1a80cd8aaea15b955a48e292c406135/src/wp-includes/post.php#L293-L294

Are they necessary at all to include? At least need to be updated.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta May 7, 2019

Author Member

I agree I don't think comments are required in this situation and I removed them.

@jorgefilipecosta jorgefilipecosta force-pushed the add/wordpress-area-cpt branch from 6c278e6 to 13c93a3 May 7, 2019

@aduth

aduth approved these changes May 8, 2019

Copy link
Member

left a comment

I left a related comment on the Widgets RFC at #14812 (comment) which, if considered, might call into question whether this post type be necessary at all.

That being said, I'd like to see some iterations in this space, and the proposed implementation looks in good shape for the current proposal.

'public' => false,
'show_ui' => false,
'show_in_menu' => false,
'show_in_rest' => true,

This comment has been minimized.

Copy link
@aduth

aduth May 8, 2019

Member

I'd also wonder if it's worth considering to include in an experimental namespace, like at #15015

@jorgefilipecosta jorgefilipecosta force-pushed the add/wordpress-area-cpt branch from 13c93a3 to 1358940 May 8, 2019

@jorgefilipecosta jorgefilipecosta merged commit 5924233 into master May 8, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@jorgefilipecosta jorgefilipecosta deleted the add/wordpress-area-cpt branch May 8, 2019

@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 10, 2019

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