-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Block editor: localize search suggestions' post type in LinkControl component (#21377) #58638
base: trunk
Are you sure you want to change the base?
Block editor: localize search suggestions' post type in LinkControl component (#21377) #58638
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Thank you for the PR. Let's sync up with #58458 as that also has a requirement to improve the Media search results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! It is working as intended for me when testing French and Spanish. I left a few comments and labeled this since it's making REST API changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR. I left some queries.
lib/compat/wordpress-6.6/class-wp-rest-media-search-handler-gutenberg.php
Outdated
Show resolved
Hide resolved
* @param \WP_REST_Request $request Rest request object. | ||
* @return string Human-readable label for the search result's post type. | ||
*/ | ||
function _gutenberg_get_search_result_label_field( $result_object, $field_name, $request ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to put these functions into a directory/file structure that mirrors the intended Core target files? If it is then it will help when it comes to merge for WP 6.6.
If not then an @core-merge
comment explaining how this code should be merged into WP Core for 6.6 would be super helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've move the new media handler to the intended directory in core.
Regarding the addition of the new label
property, I'm not sure how this should be handle. The changes touch existing classes in Core and we would need to copy the existing classes on the wp-includes/rest-api/search
folder to do the changes. This seems a lot to me.
I kept the register_rest_field
in gutenberg and I added a condition before to check if the field is present or not.
I've opened a PR in Core with the intended changes in the REST API link to this PR WordPress/wordpress-develop#6112. I can move those changes in gutenberg if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to be unavailable for a few days but will check back in here as soon as I get a chance. Thank you for your work here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened a PR in Core with the intended changes in the REST API link to this PR WordPress/wordpress-develop#6112. I can move those changes in gutenberg if needed.
The Core PR seems to contain other handlers as well and is still draft.
I think a good option would be to land the Media Search endpoint in Gutenberg first and allow it to stablise there whilst we solicit reviews from Core folks on your Core PR.
It's really great that you have the Core PR up early. Let's see if we can get some eyes on it. I'm going to ping @ellatrix who is Editor Tech Lead for WordPress 6.6 so she is aware.
38eed78
to
7a383f8
Compare
@getdave I rebased the branch against trunk to fix the conflict and redo manual testing to ensure it still work as expected. |
On my list to review this week 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/compat/wordpress-6.6/rest-api/search/class-wp-rest-media-search-handler.php
Show resolved
Hide resolved
…omponent (WordPress#21377) - Add a new media search handler to standardize the response shape for all search request in the editor - Add a new REST field `label` in the search-result schema. This field contains the localized post type name of each item in the response. - Update `getVisualTypeName` function to use the new `label` field when displaying suggestions in `LinkControl`
…avoid duplicate declarations
8d72d3e
to
88c4b71
Compare
@getdave Thanks for the review. I pushed new commits to address your comments and rebased the branch against trunk. |
Why are we adding a media search handler? |
@TimothyBJacobs This was done to unify the response format sent to the editor when doing a search. A new field The default post search handler explicitly excludes the |
@TimothyBJacobs did you had the time to check my response ? |
What?
Show localized name for post type for suggestions in
LinkComponent
Why?
Currently, suggestions are always displayed with their post type in English.
How?
label
in the search-result schema. This field contains the localized post type name of each item in the response.getVisualTypeName
function to use the newlabel
field when displaying suggestions inLinkControl
Testing Instructions
In the WordPress admin, using a custom locale (other than English)
In the results list, the post type next to each item should be displayed in the correct locale.
Screenshots or screencast
Other notes
If this solution is approved, we'll need to open a trac ticket in Core to add the new media search handler and add the new
label
field. These changes in the REST are currently polyfilled in this PR.