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

Feat: Adds the editing capability to the search title. #61508

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

Conversation

vipul0425
Copy link
Contributor

What?

Fixes issue #60701
This PR adds the editing capability to the search results title block.

Why?

How?

  • Adds the richtext component to make the title editable.

Testing Instructions

  1. Open the Site editor and edit the search template.
  2. There you will find the "Search Results Title" Block.
  3. You should be able to add and edit the text there.

Testing Instructions for Keyboard

Nil

Screenshots or screencast

compressed.screencast.1.mp4

Copy link

github-actions bot commented May 8, 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: vipul0425 <vipulgupta003@git.wordpress.org>
Co-authored-by: anton-vlasenko <antonvlasenko@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
Co-authored-by: paaljoachim <paaljoachim@git.wordpress.org>

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

Copy link

github-actions bot commented May 8, 2024

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Technical Prototype, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: .

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

Copy link

github-actions bot commented May 8, 2024

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @vipul0425! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label May 8, 2024
@jameskoster
Copy link
Contributor

First of all, this is a good PR and would be a welcome addition I think. But it does highlight some confusing aspects of this block that we should consider fixing.

Here's what I see in the editor when the 'Show search term' option is toggled off:

Screenshot 2024-05-09 at 14 20 04

The styling of the text and the caret position suggest this is a placeholder, but that's not the case—it appears on the frontend regardless:

Screenshot 2024-05-09 at 14 20 51

Contrast that with what you experience when 'Show search term' is toggled on:

Screenshot 2024-05-09 at 14 22 03

Now 'Search results for' and 'suffix' look like placeholders, and indeed that's what they are—they do not appear on the frontend when left empty:

Screenshot 2024-05-09 at 14 23 29

Imo it would be good to fix this inconsistency before merging.

@@ -38,13 +38,23 @@ function render_block_core_query_title( $attributes ) {
}
}
if ( $is_search ) {
$title = __( 'Search results' );
$title = isset( $attributes['searchResultsTerm'] ) && ! empty( $attributes['searchResultsTerm'] ) ? $attributes['searchResultsTerm'] : __( 'Search results' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling isset() is not needed since empty() already checks if the property is set. Calling !empty( $attributes['searchResultsTerm'] ) is equivalent to calling isset( $attributes['searchResultsTerm'] ) && $attributes['searchResultsTerm'].

Suggested change
$title = isset( $attributes['searchResultsTerm'] ) && ! empty( $attributes['searchResultsTerm'] ) ? $attributes['searchResultsTerm'] : __( 'Search results' );
$title = ! empty( $attributes['searchResultsTerm'] ) ? $attributes['searchResultsTerm'] : __( 'Search results' );

Copy link
Contributor Author

@vipul0425 vipul0425 May 9, 2024

Choose a reason for hiding this comment

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

Thanks @anton-vlasenko. I have fixed it in the latest commit 🙇

@vipul0425
Copy link
Contributor Author

First of all, this is a good PR and would be a welcome addition I think. But it does highlight some confusing aspects of this block that we should consider fixing.

Here's what I see in the editor when the 'Show search term' option is toggled off:

Screenshot 2024-05-09 at 14 20 04 The styling of the text and the caret position suggest this is a placeholder, but that's not the case—it appears on the frontend regardless: Screenshot 2024-05-09 at 14 20 51 Contrast that with what you experience when 'Show search term' is toggled on: Screenshot 2024-05-09 at 14 22 03 Now 'Search results for' and 'suffix' look like placeholders, and indeed that's what they are—they do not appear on the frontend when left empty: Screenshot 2024-05-09 at 14 23 29 Imo it would be good to fix this inconsistency before merging.

Thanks, @jameskoster, for pointing this out. I've addressed the inconsistency in the latest commit.

@t-hamano t-hamano self-requested a review May 12, 2024 15:34
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

@vipul0425 Thanks for the PR!

I think the important thing here is that the "Archive Title" block and the "Search Results Title" block exist as variations of the block called "Query Title" block. Similarly, the text of the Archive Title block cannot be edited at all.

Is there a request to be able to edit the prefix and suffix text in the Archive Title block as well? If there is, it may be better to avoid adding attribute names that depend on specific variations such as searchResultsTerm and searchResultsTermSuffix. More generic attribute names like prefix and suffix might be useful if we want to apply this approach to the Archive Title block in the future.

Another important point is that the term "Search Results for:" is not guaranteed to appear before the search term in all languages. For example, in Japanese Search results for "%s" is translated as 「%s」の検索結果.

In order to advance this PR, I would be happy if we could have a more detailed discussion and find the ideal approach.

I'll also ping @jorgefilipecosta and @carolinan who have worked on improving this block in the past.

@t-hamano t-hamano added [Type] Enhancement A suggestion for improvement. [Block] Query Title Affects the Query Title Block labels May 12, 2024
@vipul0425
Copy link
Contributor Author

Hi @t-hamano,

Thanks for reviewing this. Regarding the "Archive Title," I believe it should be editable, but as there was no request for adding that, I updated only for the Search Title block. I could only find this issue from the past referring to the update in Archive-title #44158.

Regarding using generic terms for prefixes and suffixes, do we want to share the same prefix and suffix across the archive and search title like the showPrefix attribute? I think we should keep them separate though we can club both the prefixes (archive and search title) under a common attribute object like "prefix." Let me know what you think about this.

Thanks for letting me know about "Search Results for:" in different language scenarios. We can use a generic placeholder value like "prefix." Let me know if you have any better suggestions.

Thanks 🙇

@paaljoachim
Copy link
Contributor

paaljoachim commented May 15, 2024

Is there a request to be able to edit the prefix and suffix text in the Archive Title block as well? If there is, it may be better to avoid adding attribute names that depend on specific variations such as searchResultsTerm and searchResultsTermSuffix. More generic attribute names like prefix and suffix might be useful if we want to apply this approach to the Archive Title block in the future.

This sounds like a very good idea!
@vipul0425 If you can add the same generic prefix and suffix to the Archive Title block in addition to the Search Results Title block that would be very helpful.

I will update the original issue to reflect a change to both the Search Results Title block and the Archive Title block.

@t-hamano
Copy link
Contributor

Sorry for the late reply. I was thinking of an ideal solution to this problem, but it might be more complicated than I thought 😅

What I think is the ideal approach is as follows:


The default state of the block has Prefix and suffix placeholders and displays only the search term.

default

Users can reproduce the same title by entering text in Prefix and Suffix.

default-updated

When "Show search term in title" is off, it will display a placeholder with the placeholder "Search results". This is managed internally as the prefix attribute. This placeholder is different from the normal prefix and suffix and is meant to be the default text.

default-show-search-term-off

If this approach makes sense, block migration is required to preserve the appearance of existing blocks. We will need to consider the following two patterns:

If "Show search term in title" is enabled

image

Assign Search results for: “ to the prefix attribute and to the suffix attribute.

If "Show search term in title" is disabled

image

Assign Search results to the prefix attribute.


What do you think about this approach?

@paaljoachim
Copy link
Contributor

paaljoachim commented May 23, 2024

Hey @t-hamano Aki

Search

This is what I read.

An example
User searches for course.

Default state is when the toggle Show search term in title is on. The Search page will show:

Search results for: “course”

When the toggle search term in title is off. The Search page will show:

Search results

Adding a Prefix to adjust the Search results / Search results for: “ —> will be very helpful.
Adding a Suffix to be able to add text after the search term -> will also be very helpful.

This means.
When the default toggle Show search term in title is on.
One would be able to change Search results for: (Prefix) and (Suffix) to example You are looking for course which we offer.

You are looking for -> is then the Prefix.
which we offer. -> is then the Suffix.

When the toggle Show search term in title is off.
One would be able to change the Search results (Prefix) to something else. As the search term is not inserted there is no need for a Suffix.

Conclusion.
When the toggle is on. It uses Prefix and Suffix to be able to add text before and after a specific search term.
When the toggle is off. It would only use the Prefix to add text/title to the Search page.

This sounds like a good approach!


Archive title

Default toggle on: Show archive type in title. (Archive type: Name)

Example. A category named course. On the frontend it will show Category: course

Toggle is off. (Archive title)
On the frontend it will then show the category name: course

Since text is automatically inserted when the toggle is on or off.
Be it Category: course or only course it would be helpful to have Prefix and Suffix for both options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Query Title Affects the Query Title Block First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants