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

Support the blockTypes and viewport_width props for remote patterns fetched from Pattern Directory #3967

Conversation

ntsekouras
Copy link

@ntsekouras ntsekouras commented Feb 2, 2023

Trac ticket: https://core.trac.wordpress.org/ticket/57611

We need to add the support for block_types and viewport_width props for remote patterns fetched from Pattern Directory.

With this support, we can fetch remote patterns and use them properly as suggestions where needed.

The addition of block_types field in Pattern Directory API was made here: WordPress/pattern-directory#111

GB PR: WordPress/gutenberg#47677

props @ryelle

Before

Screen.Recording.2023-02-02.at.11.35.59.AM.mov

After

Screen.Recording.2023-02-02.at.11.36.55.AM.mov

Testing Instructions

  1. Insert a Heading block
  2. Open the block switcher menu
  3. Observe that there is a pattern suggested, coming from Pattern Directory

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@hellofromtonya hellofromtonya force-pushed the add/support-block_types-viewport_width-in-remote-patterns branch from 605432f to cf2cae5 Compare February 2, 2023 20:34
* @param array $pattern Pattern as returned from the Pattern Directory API.
* @return array Normalized pattern.
*/
function _normalize_remote_pattern( $pattern ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A search of the wp.org repo shows:

The name follows the naming patterns in this file.

Note: There could be a risk for a naming conflict in the wild.

Copy link
Author

Choose a reason for hiding this comment

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

Open to any suggestions :)

Choose a reason for hiding this comment

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

Would _normalize_remote_block_pattern() help? The other functions in this file refer to "block" and/or "theme" in their names. (Also, there are 0 plugins and 0 themes using this.)

Copy link
Author

Choose a reason for hiding this comment

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

I renamed to _normalize_remote_block_pattern.

Copy link
Member

Choose a reason for hiding this comment

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

See #3967 (review), we should not be using underscore prefixes in Core. This is something that I was already advised by seasoned committers several years ago. While occasionally it gets overlooked, it's not a good practice. The recommendation is to simply mark the function with @access private.

I've updated this in 6f39a68.

Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

  • Confirmed it includes changes from GB PR ✅
  • Test Report confirms it works ✅
  • Code meets Core's coding standards ✅

LGTM and ready for commit 👍

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@ntsekouras One naming nit-pick; otherwise this is good to go.

src/wp-includes/block-patterns.php Outdated Show resolved Hide resolved
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Made the small naming update, using the wp_ prefix.

@felixarntz
Copy link
Member

@felixarntz felixarntz closed this Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants