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

Block editor: localize search suggestions' post type in LinkControl component (#21377) #58638

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Conversation

petitphp
Copy link
Contributor

@petitphp petitphp commented Feb 3, 2024

What?

Show localized name for post type for suggestions in LinkComponent

Why?

Currently, suggestions are always displayed with their post type in English.

How?

  • 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

Testing Instructions

In the WordPress admin, using a custom locale (other than English)

  1. Create a few posts / pages / attachment
  2. In a new post, insert a Button block
  3. Select the link option in the block toolbar and search for suggestion

In the results list, the post type next to each item should be displayed in the correct locale.

Screenshots or screencast

patch-21377

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.

Copy link

github-actions bot commented Feb 3, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: petitphp <petitphp@git.wordpress.org>
Co-authored-by: MaggieCabrera <onemaggie@git.wordpress.org>
Co-authored-by: getdave <get_dave@git.wordpress.org>
Co-authored-by: TimothyBJacobs <timothyblynjacobs@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@getdave getdave added [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) [Type] Enhancement A suggestion for improvement. labels Feb 5, 2024
@getdave
Copy link
Contributor

getdave commented Feb 6, 2024

Thank you for the PR. Let's sync up with #58458 as that also has a requirement to improve the Media search results.

@MaggieCabrera MaggieCabrera added the REST API Interaction Related to REST API label Feb 6, 2024
Copy link
Contributor

@MaggieCabrera MaggieCabrera left a 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.

lib/class-wp-rest-media-search-handler-gutenberg.php Outdated Show resolved Hide resolved
lib/rest-api.php Outdated Show resolved Hide resolved
Copy link
Contributor

@getdave getdave 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 the PR. I left some queries.

* @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 ) {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

@petitphp
Copy link
Contributor Author

@getdave I rebased the branch against trunk to fix the conflict and redo manual testing to ensure it still work as expected.

@getdave
Copy link
Contributor

getdave commented Apr 22, 2024

On my list to review this week 👍

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Testing this it worked as described. I think it's good to land in Gutenberg and allow it to stabilise.

Screen Shot 2024-04-26 at 15 06 33

A few nits which it would be good to clean up first though.

You can then work with WP 6.6 Tech Leads to iterate on getting it merged to Core early in this release cycle.

@getdave getdave added the Needs PHP backport Needs PHP backport to Core label Apr 26, 2024
…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`
@petitphp petitphp force-pushed the fix/localized-link-suggestions-posttype branch from 8d72d3e to 88c4b71 Compare May 17, 2024 13:35
@petitphp
Copy link
Contributor Author

@getdave Thanks for the review.

I pushed new commits to address your comments and rebased the branch against trunk.

@TimothyBJacobs
Copy link
Member

Why are we adding a media search handler?

@petitphp
Copy link
Contributor Author

@TimothyBJacobs This was done to unify the response format sent to the editor when doing a search.

A new field label was added to the search handler to provide a localized version of type/subtype, but we currently use the dedicated REST media endpoint to handle the search, and I didn't want to add a new field related to the editor only for this endpoint.

The default post search handler explicitly excludes the attachment postType, so I thought it was better to introduce a dedicated search handler.

@petitphp
Copy link
Contributor Author

petitphp commented Jul 1, 2024

@TimothyBJacobs did you had the time to check my response ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) Needs PHP backport Needs PHP backport to Core REST API Interaction Related to REST API [Type] Enhancement A suggestion for improvement.
4 participants