Skip to content

Announce number of search results in title#2688

Draft
stephanieleary wants to merge 11 commits into
vufind-org:devfrom
stephanieleary:search-titles
Draft

Announce number of search results in title#2688
stephanieleary wants to merge 11 commits into
vufind-org:devfrom
stephanieleary:search-titles

Conversation

@stephanieleary
Copy link
Copy Markdown
Contributor

@stephanieleary stephanieleary commented Jan 30, 2023

Adjusts the search results page <title> to include errors, no results, or the number of results shown vs. total found. This lets screen reader users know right away whether their search was successful, so they can immediately move to the search landmark to edit if necessary.

Repeats some code from search/controls/showing.phtml, which may not be ideal. Since those translated strings may include markup, strip_tags() is used to clean them up for $headTitle.

Adjusts the search results page title to include errors, no results, or the number of results shown vs. total found. This lets screen reader users know right away whether their search was successful, so they can immediately move to the search landmark to edit if necessary.

Repeats some code from search/controls/showing.phtml, which may not be ideal.
Comment thread themes/bootstrap3/templates/search/results.phtml Outdated
Adjusts the search results page title to include errors, no results, or
the number of results shown vs. total found. This lets screen reader
users know right away whether their search was successful, so they can
immediately move to the search landmark to edit if necessary.

Rewritten to take advantage of $this->layout()->srmessage.

This reverts commit 532de9c.
Copy link
Copy Markdown
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @stephanieleary! It looks like there may be some minor whitespace issues here that are causing style checks to fail. You can probably fix everything with vendor/bin/phing php-cs-fixer. Once the build is passing CI, I'll take a closer look. In the meantime, let me know if I can help with anything, if the style checks are being stubborn. :-)

In the previous iteration, the page title worked but the breadcrumbs disappeared. This restores $headTitle at the top of the file for use in breadcrumbs. It will be reassigned at the end, after $this->layout()->srmessage has been updated in case of empty result sets.
@stephanieleary
Copy link
Copy Markdown
Contributor Author

@demiankatz I ran php-cs-fixer on this, and I can't tell why it's still failing. I'm happy to adjust a setting or something to make it work!

@demiankatz
Copy link
Copy Markdown
Member

@stephanieleary, I made a few minor style adjustments and the build is passing now. (I think the main thing that was causing the failure was a blank space on the end of a <?php line which was violating our "no extra end-of-line whitespace" rule. Not sure why php-cs-fixer failed to sort this out for you, though. In any case, I'm about out of time for this week, but I'll give this a closer look next week (along with your other pending PR work). Have a good weekend in the meantime!

@demiankatz
Copy link
Copy Markdown
Member

@stephanieleary, I took a closer look at this today, and made a few minor adjustments to eliminate unnecessary whitespace changes (thus focusing the diffs in this PR on what's actually changing).

My biggest concern is the possible impact this will have on browser tab/title bar titles. For example, see these two tabs:

image

The one on the left is the current dev branch; the one on the right is the same search using this branch. As you can see, the actual search term is now pushed out of visibility. I think this could be a nuisance if someone has multiple searches open at once and wants to easily navigate between them. I realize there's a trade-off between simplicity of code and reusing existing data vs. having nuanced control over how titles are formatted. I just wonder if this issue might be significant enough to justify a slightly more complex approach. One compromise might be to make use of the showing_results_of_html translation string, and append that onto the existing search terms. Then we could use a title like test: showing results x-y of z instead of showing results x-y of z for search test, which puts the most important differentiating factor first.

I'd be interested to hear opinions from others like @crhallberg and @ckaz.

@crhallberg
Copy link
Copy Markdown
Contributor

I imagine that some power users would get benefits from seeing the search term via @demiankatz's rephrasing.

Comment thread themes/bootstrap5/templates/search/results.phtml
@stephanieleary
Copy link
Copy Markdown
Contributor Author

I agree that the new wording pushes the search term too far from the beginning of the title. I was hoping to find a good solution for this without adding new translation strings, but perhaps there's no way around that...?

@demiankatz
Copy link
Copy Markdown
Member

@stephanieleary, my suggestion was to concatenate the search query with the existing showing_results_of_html string; I think that might give you a reasonable result without new translation. Of course, I'm afraid that that sort of undermines the clever solution we came up with here for avoiding redundant code.

Additionally, if it feels like too much of a hack to do string concatenation instead of pure translation, we could use the command line tools for combining language strings together to introduce a new language string for this purpose that utilizes existing translations. That might be a "best of both worlds" approach where we have a single overrideable language string for the page title, but we set it to a default that avoids the need to bother the translation team for new work. If you think that's worth doing, I'm happy to help set it up (or can point you to the specifics if you'd like to learn how to do it yourself).

@stephanieleary
Copy link
Copy Markdown
Contributor Author

In general, I'm wary of concatenating strings after translation--I don't think we can be certain that the order will make sense. I'd love to get some feedback on the idea from translators.

@demiankatz
Copy link
Copy Markdown
Member

@stephanieleary, I share your wariness, though in this case, if we're concatenating two discrete entities (the search terms, and a message about those search terms), I think the risk is lessened -- there's not really a semantic meaning to the order, apart from the fact that we want the most useful part to come first for display purposes. But we could certainly mitigate the problem by creating a new translation string as I proposed -- then we can come up with a reasonable default but easily override it on a language-by-language basis as needed.

If you'd like to give it a shot, try this command at the command line:

php $VUFIND_HOME/public/index.php language/addusingtemplate query_plus_showing_results_of_html "%%query%% :: ||showing_results_of_html||"

@demiankatz demiankatz added this to the 9.1 milestone May 2, 2023
@demiankatz
Copy link
Copy Markdown
Member

@stephanieleary, this PR is on the list of 9.1 milestone work to be discussed on next week's Community Call, so I went ahead and updated it by merging in the latest dev branch. Hopefully this will get us a green checkmark again. Please let me know when you're ready to continue discussing this work!

@demiankatz
Copy link
Copy Markdown
Member

@stephanieleary, we had a discussion on today's Community Call about reassigning PRs to the 10.0 release if they are unlikely to be completed before September (since we're hoping to stabilize the code about a month in advance of our anticipated October release). Do you think you'll have time to move this forward in the next couple of months, or should I push this forward to the 10.0 milestone?

@ckaz
Copy link
Copy Markdown
Contributor

ckaz commented Sep 7, 2023

@stephanieleary, I took a closer look at this today, and made a few minor adjustments to eliminate unnecessary whitespace changes (thus focusing the diffs in this PR on what's actually changing).

My biggest concern is the possible impact this will have on browser tab/title bar titles. For example, see these two tabs:

image

The one on the left is the current dev branch; the one on the right is the same search using this branch. As you can see, the actual search term is now pushed out of visibility. I think this could be a nuisance if someone has multiple searches open at once and wants to easily navigate between them. I realize there's a trade-off between simplicity of code and reusing existing data vs. having nuanced control over how titles are formatted. I just wonder if this issue might be significant enough to justify a slightly more complex approach. One compromise might be to make use of the showing_results_of_html translation string, and append that onto the existing search terms. Then we could use a title like test: showing results x-y of z instead of showing results x-y of z for search test, which puts the most important differentiating factor first.

I'd be interested to hear opinions from others like @crhallberg and @ckaz.

Apologies for being so late on this: Across all the library catalogs we do, we have but for 2 or 3 exceptions opted for leaving the "showing x to y of z results" out entirely. The rationale being that

  • the number-of-results-in-list-select element was deemed sufficient for making clear that only 20/50... results are currently shown.
  • the total number of results tends to be misleading in searches with lots of results because one cannot actually browse to result 23824 (or whatever)
  • the result numbers listed in the facets were considered more accurate and useful than the general, unspecific announcement of hits

So, even if this is not directly helpful for this ticket, I think it might be worth (re-)considering the informative value of the entire element.

However, I fully agree with the original intention of announcing the total number in the page title, so people can adjust their search terms immediately.

@demiankatz demiankatz modified the milestones: 9.1, 10.0 Oct 3, 2023
@demiankatz
Copy link
Copy Markdown
Member

I'm moving this to the 10.0 milestone, as we're getting close to the 9.1 code freeze, and I don't think this is going to get done in time. Of course, happy to keep working on it regardless whenever you're ready!

EreMaijala pushed a commit to EreMaijala/vufind that referenced this pull request Oct 20, 2023
@demiankatz demiankatz modified the milestones: 10.0, 10.1 May 7, 2024
@demiankatz
Copy link
Copy Markdown
Member

I'm moving this forward to the 10.1 milestone due to inactivity. Happy to revive any time if @stephanieleary has time.

@crhallberg
Copy link
Copy Markdown
Contributor

crhallberg commented Aug 12, 2024

Now that we're past the v10 release, I think it's time to add the new translation string, which seems like the simplest, most accessible, and most customizable result. I've had issues in the past with the concatenation of two different language strings. That kind of what I will agree is a hack only leads to poorly-structured phrasing in some of the languages. The only alternative I can come up with is a pseudo translation string in config, a solution I'm not too pleased with.

@demiankatz demiankatz modified the milestones: 10.1, 11.0 Oct 8, 2024
@demiankatz
Copy link
Copy Markdown
Member

I'm moving this to the 11.0 milestone as discussed at the 2024 Summit roadmapping session.

@demiankatz demiankatz modified the milestones: 11.0, 11.1 Jul 1, 2025
@demiankatz
Copy link
Copy Markdown
Member

Reassigning milestone as per discussion on the 1 July Community Call.

@demiankatz
Copy link
Copy Markdown
Member

I have merged the dev branch into this PR and resolved conflicts.

To highlight what is changed here, some examples:

Empty search result page title, before change: Search Results - noresultstobefound :: Library Catalog
Empty search result page title, after change: Your search - noresultstobefound - did not match any resources. :: Library Catalog

Non-empty search result page title, before change: Search Results - test :: Library Catalog
Non-empty search result page title, after change: Showing 1 - 20 results of 69 :: Library Catalog

While I do think there is some value in incorporating result counts into the page title, I think losing the search query is actually a significant regression. If I had to keep track of multiple browser tabs, the search terms I had entered would be more valuable to me than the result counts.

Would it make sense to aim for a hybrid approach, like these?

Proposed empty search title: Search Results - noresultstobefound :: No Results! :: Library Catalog
Proposed non-empty search title: Search Results - test :: Showing 1 - 20 results of 69 :: Library Catalog

Or is that too verbose? Is there a better order for the title elements?

I'd welcome input from @metageeky and others. I don't think it will be hard to implement this, once we've decided exactly what we should implement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants