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 context properties to block types REST endpoint #22686

Merged
merged 15 commits into from
Jun 19, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented May 27, 2020

This pull request seeks to add block context properties to the block types REST controller originally implemented in #21065. Block context documentation was created as part of #21925, but not incorporated into the existing Block Type RFC document. It has been included here alongside the necessary revisions to include the properties in the REST controller.

Status: The implementation should work well enough, though it may warrant being considered blocked by one or both of:

Testing Instructions:

Ensure tests pass:

./packages/env/bin/wp-env run composer 'composer install'
npm run test-unit-php

Test scenario from #21467 is helpful as well:

Given that this pull request only ports the Post Title experimental full-site editing block, testing can be a bit cumbersome at the moment, especially if you've not yet defined templates for full-site editing mode.

My recommendation for testing is as follows:

  1. Navigate to Gutenberg > Experiments
  2. Enable "Full Site Editing" and click "Save Changes"
  3. Navigate to Posts > Add New
  4. Insert a Post Title block
  5. Observe that the block inherits the title of the post and updates if you change the title
  6. Preview the post in a separate tab
    • Note: If you don't have templates on your site, you may see a message "No template found". This is expected, and can be disregarded for now.
  7. Navigate to Gutenberg > Experiments
  8. Disable "Full Site Editing" and click "Save Changes"
  9. In your separate preview tab, refresh the page.
  10. Observe that the Post Title block displays the title of the current post

@aduth aduth added [Type] Enhancement A suggestion for improvement. REST API Interaction Related to REST API labels May 27, 2020
@aduth aduth requested review from spacedmonkey and gziolo May 27, 2020 22:17
@github-actions
Copy link

github-actions bot commented May 27, 2020

Size Change: +218 B (0%)

Total Size: 1.12 MB

Filename Size Change
build/block-editor/index.js 106 kB +133 B (0%)
build/block-editor/style-rtl.css 10.7 kB +6 B (0%)
build/block-editor/style.css 10.7 kB +5 B (0%)
build/block-library/editor-rtl.css 7.87 kB +6 B (0%)
build/block-library/editor.css 7.87 kB +5 B (0%)
build/block-library/index.js 129 kB +3 B (0%)
build/components/style-rtl.css 15.9 kB +11 B (0%)
build/components/style.css 15.8 kB +11 B (0%)
build/edit-post/style-rtl.css 5.5 kB +4 B (0%)
build/edit-post/style.css 5.5 kB +5 B (0%)
build/editor/style-rtl.css 3.81 kB +4 B (0%)
build/editor/style.css 3.81 kB +3 B (0%)
build/format-library/style-rtl.css 547 B +5 B (0%)
build/format-library/style.css 548 B +5 B (0%)
build/list-reusable-blocks/style-rtl.css 450 B +4 B (0%)
build/list-reusable-blocks/style.css 451 B +4 B (0%)
build/nux/style-rtl.css 663 B +1 B
build/nux/style.css 660 B +3 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.26 kB 0 B
build/block-directory/style-rtl.css 937 B 0 B
build/block-directory/style.css 937 B 0 B
build/block-library/style-rtl.css 8 kB 0 B
build/block-library/style.css 8.01 kB 0 B
build/block-library/theme-rtl.css 730 B 0 B
build/block-library/theme.css 732 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 196 kB 0 B
build/compose/index.js 9.6 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.17 kB 0 B
build/edit-navigation/index.js 8.26 kB 0 B
build/edit-navigation/style-rtl.css 1.02 kB 0 B
build/edit-navigation/style.css 1.02 kB 0 B
build/edit-post/index.js 303 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 3.03 kB 0 B
build/edit-site/style.css 3.03 kB 0 B
build/edit-widgets/index.js 9.33 kB 0 B
build/edit-widgets/style-rtl.css 2.43 kB 0 B
build/edit-widgets/style.css 2.43 kB 0 B
build/editor/editor-styles-rtl.css 468 B 0 B
build/editor/editor-styles.css 469 B 0 B
build/editor/index.js 44.8 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 kB 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 788 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

* Type: `object`
* Optional
* Localized: No
* Property: `providesContext`
Copy link
Member

Choose a reason for hiding this comment

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

Does this property map to provides_context in PHP?

Copy link
Member Author

Choose a reason for hiding this comment

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

Does this property map to provides_context in PHP?

Technically there's no mapping today, but I've mentioned it in the original comment as a potential blocking decision:

A decision at #22519 (comment) in regards to registering block types using snake-cased provides_context, rather than camel-cased providesContext.

* Type: `string[]`
* Optional
* Localized: No
* Property: `context`
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this naming. Context is already in use in the REST API and it reusing this naming is super confusing.
If I make a request to /wp/v2/block-type/?context=edit what do I get. The request in an edit context and block files by context === edit. Confusing. Can we use another name here?

Copy link
Member

Choose a reason for hiding this comment

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

How about the naming block_context?

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, it's a completely different concept from what REST API uses. It didn't occur to me earlier that the name might be too general, it's more like:

  • dataContext or externalAttributes
  • providesAttributes

I'm curious to hear how @aduth views it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd consider "context" a fairly generic term of the likes of "type", and the relationship between blocks and the REST API is incidental enough that—outside this specific collision—I can't imagine there'd be a high likelihood for confusion of the two.

Conversely, as I understand from some of early discussions like in #19685, there's an intentional overlap of the name it and the concept of Context in React which it mimics (and uses for implementation in the client).

I'm not especially attached to the name, but it wasn't really the intent of the pull request to be introducing the concept, moreso to bring this document into alignment with what had already been implemented across discussions and iterations of #19685, #19572, and #21467.

There's some interesting merit to thinking about this in terms of "attributes provided from some ancestor" and potentially naming around that, though even that may restrict some options for how context could expand in the future (see #19685 (comment), #19685 (comment)).

Since there's far more impact to a decision than what's affected here, I'd rather weigh it in its own dedicated space (an issue or editor meeting discussion).

Copy link
Member

Choose a reason for hiding this comment

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

Context is already in use in core and to add this here would be really confusing. Please can we just rename it?

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Thank you for adding missing support for the block context. Overall it looks good. We just need to decide whether context is a good fit here 👍

@aduth
Copy link
Member Author

aduth commented May 28, 2020

Per #22686 (comment) and #22686 (comment), I documented default values in d255bb0.

A couple additional notes:

  • There's no precedent elsewhere in the document for specifying default values. That said, it seems like it would be a good thing to do, assuming there are sensible defaults, like what are now provided through the REST endpoint.
  • It will also depend on effort like in Trac#48529 to ensure those default are in-fact applied.

@gziolo gziolo force-pushed the add/block-types-context-fields branch from d255bb0 to d438cac Compare June 15, 2020 15:58
@gziolo
Copy link
Member

gziolo commented Jun 15, 2020

@spacedmonkey, I applied changes to unblock this PR per your feedback. Does it work as is?

We still need to decide on about the block registry:

A decision at #22519 (comment) in regards to registering block types using snake-cased provides_context, rather than camel-cased providesContext.

I believe it can be tackled separately though. From the perspective of the REST API endpoint. everything seems fine.

@gziolo gziolo self-assigned this Jun 15, 2020
@spacedmonkey
Copy link
Member

@aduth Should we be able to filter the block by these context? So add ?use_context=widgets? It would be a nice tweak.

@gziolo gziolo force-pushed the add/block-types-context-fields branch from e098182 to 18002bb Compare June 19, 2020 11:56
@gziolo gziolo requested a review from epiqueras June 19, 2020 11:58
@gziolo gziolo merged commit c33a5a5 into master Jun 19, 2020
@gziolo gziolo deleted the add/block-types-context-fields branch June 19, 2020 13:58
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 19, 2020
@mcsf
Copy link
Contributor

mcsf commented Jun 19, 2020

gziolo merged commit

🎉

Nice collab here.

@gziolo gziolo added the Backport to WP 6.6 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jun 22, 2020
nylen pushed a commit to nylen/wordpress-develop-svn that referenced this pull request Jun 22, 2020
New block context related fields were added as part of WordPress/gutenberg#22686. This changest backports them to WP_Block_Type class.

Props aduth, spacedmonkey, mcsf, epiqueras.
Fixes #47656.  



git-svn-id: https://develop.svn.wordpress.org/trunk@48117 602fd350-edb4-49c9-b593-d223f7449a82
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Jun 22, 2020
New block context related fields were added as part of WordPress/gutenberg#22686. This changest backports them to WP_Block_Type class.

Props aduth, spacedmonkey, mcsf, epiqueras.
Fixes #47656.  



git-svn-id: https://develop.svn.wordpress.org/trunk@48117 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Jun 22, 2020
New block context related fields were added as part of WordPress/gutenberg#22686. This changest backports them to WP_Block_Type class.

Props aduth, spacedmonkey, mcsf, epiqueras.
Fixes #47656.  


Built from https://develop.svn.wordpress.org/trunk@48117


git-svn-id: http://core.svn.wordpress.org/trunk@47886 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott pushed a commit to gMagicScott/core.wordpress-mirror that referenced this pull request Jun 22, 2020
New block context related fields were added as part of WordPress/gutenberg#22686. This changest backports them to WP_Block_Type class.

Props aduth, spacedmonkey, mcsf, epiqueras.
Fixes #47656.  


Built from https://develop.svn.wordpress.org/trunk@48117


git-svn-id: https://core.svn.wordpress.org/trunk@47886 1a063a9b-81f0-0310-95a4-ce76da25c4cd
This was referenced Jun 24, 2020
donmhico pushed a commit to donmhico/wordpress-develop that referenced this pull request Jun 26, 2020
New block context related fields were added as part of WordPress/gutenberg#22686. This changest backports them to WP_Block_Type class.

Props aduth, spacedmonkey, mcsf, epiqueras.
Fixes #47656.  



git-svn-id: https://develop.svn.wordpress.org/trunk@48117 602fd350-edb4-49c9-b593-d223f7449a82
@gziolo gziolo removed the Backport to WP 6.6 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REST API Interaction Related to REST API [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants