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

Add a mini pager option to Search module results. #6083

Closed
leeksoup opened this issue Apr 26, 2023 · 18 comments · Fixed by backdrop/backdrop#4424
Closed

Add a mini pager option to Search module results. #6083

leeksoup opened this issue Apr 26, 2023 · 18 comments · Fixed by backdrop/backdrop#4424

Comments

@leeksoup
Copy link

leeksoup commented Apr 26, 2023

Description of the need

All other pagers on my site (mostly generated using Views) have the "mini" pager. It's frustrating to have this one page use only the "full" style pager, and it generates a LOT of buttons which looks pretty silly.

Proposed solution

Ideally, the search module config would have an option to do a "mini" pager like Views module does.

Alternatives that have been considered

I have used Views with exposed filters for search in the past (like many years ago before Drupal's search was polished) but I would prefer to use the search module because it works a lot faster.

Additional information

Desired look:
image

Current look:
image

Draft of feature description for Press Release (1 paragraph at most)

Backdrop now includes the option to use mini pagers on search results pages for a sleek appearance.


TRANSLATION UPDATE

Changed strings:

  • old: The maximum number of items indexed in each pass of a <a href="@cron">cron maintenance task</a>. If necessary, reduce this number to prevent timeouts and memory errors while indexing.
  • new: The maximum number of items indexed in each pass of a <a href="@cron">cron maintenance task</a>. If necessary, reduce this number to prevent timeouts and memory errors while indexing.
  • old2: Whether to apply a simple Chinese/Japanese/Korean tokenizer based on overlapping sequences. Turn this off if you want to use an external preprocessor for this instead. Does not affect other languages. <strong>Changing this setting will invalidate the search index.</strong>
  • new2: Whether to apply a simple Chinese/Japanese/Korean tokenizer based on overlapping sequences. Turn this off if you want to use an external preprocessor for this instead. Does not affect other languages. <strong>Changing this setting will invalidate the search index.</strong>'

New strings:

  • Pager type
  • Full pager
  • Mini pager
  • Includes "Previous"/"Next" and "First"/"Last" links, as well as numbered links for each page of the search results.
  • Shows only "Previous"/"Next" links, and indications of the current and total number of search results pages.
@yorkshire-pudding
Copy link
Member

It may also be good, though this is a nice to have rather than a must have, to allow the switch to be responsive to screen size (i.e. full pager on desktop, mini pager on mobile).

@klonos
Copy link
Member

klonos commented Apr 30, 2023

Here's an initial PR up for testing/review: backdrop/backdrop#4417

It adds the following fieldset in the "Search settings" form at admin/config/search/settings:
image

Seeking initial review/feedback on the implementation, but there's currently a problem that I would like to figure out (for which I have added a @to-do comment in the relevant code section): theme_pager() (which is being used for the current behavior of always rendering a full pager) works fine, however theme_views_mini_pager() (which I am re-using for the purposes of minimal code changes) needs a cache clear in each page of the search results in order for the mini pager to be rendered - the theme registry cache needs to be cleared specifically. I wasn't able to figure out why that is.

@klonos
Copy link
Member

klonos commented Apr 30, 2023

It may also be good, though this is a nice to have rather than a must have, to allow the switch to be responsive to screen size (i.e. full pager on desktop, mini pager on mobile).

@yorkshire-pudding I think that this is a nice idea for how pagers should be behaving in general (those in views as well). Wanna file a separate issue for this?

@klonos
Copy link
Member

klonos commented Apr 30, 2023

How to test this:

  • Install the Devel module, and enable the devel_generate submodule.
  • Generate a bunch of content (I generated 5000 nodes on my local)
  • Head to /admin/config/search/settings and reindex the site (remaining items option will do)
  • Head to /search and search for a term that will generate multiple results (in my case, searching for "refero" did it)
  • Back in /admin/config/search/settings switch between the two new options for the pager, followed by saving the form
  • Refresh the /search page to see the switch of pager type take effect.

@stpaultim
Copy link
Member

stpaultim commented May 1, 2023

Works for me. The new pager looks like this in Basis.
image

FULL PAGER:

image

@yorkshire-pudding
Copy link
Member

It may also be good, though this is a nice to have rather than a must have, to allow the switch to be responsive to screen size (i.e. full pager on desktop, mini pager on mobile).

@yorkshire-pudding I think that this is a nice idea for how pagers should be behaving in general (those in views as well). Wanna file a separate issue for this?

Added a bare bones issue here: #6094

@klonos
Copy link
Member

klonos commented May 1, 2023

Thanks @yorkshire-pudding 🙏🏼

I'm adding the "help wanted" label to see if anyone can figure out the issue with the necessity to be clearing the theme registry on every search result page in order to have the Views-provided mini pager be rendered.

@quicksketch
Copy link
Member

I posted a small suggestion to the PR: backdrop/backdrop#4417 (review)

I'll look into the registry rebuild issue.

@quicksketch
Copy link
Member

I filed a replacement PR that fixes the registry issue and renames the keys stored in config: backdrop/backdrop#4424

@klonos
Copy link
Member

klonos commented May 1, 2023

Thanks for that @quicksketch 🙏🏼 ...I had tried to avoid having to "map" the options keys to the the respective theme function name, but now that you have implemented it, it doesn't look that bad 👍🏼 ...and that one-liner fix for the theme registry cache was so simple 🤦🏼 👍🏼

Code looks good to me, so RTBC 👍🏼 ...also giving a final round of testing.

@quicksketch
Copy link
Member

I had tried to avoid having to "map" the options keys to the the respective theme function name

Yeah, I'd like to get us set up for renamespacing views_mini_pager to just mini_pager, so avoiding storing the name in config might save us another update hook later.

@klonos
Copy link
Member

klonos commented May 1, 2023

I've tested this in the PR sandbox by following the steps outlined in #6083 (comment) above. WFM, so this is good to go 👍🏼

The mini pager needs a little bit of love, so that #6089 is fixed, and also #6095.

@leeksoup
Copy link
Author

leeksoup commented May 1, 2023

@klonos and @quicksketch - thanks very much for the quick implementation and fix! I'm pulling 4424 now and testing it on my test site.

@quicksketch
Copy link
Member

Welcome @leeksoup to the Backdrop community! I've added you to our "Docs and organization" group here on GitHub so you can now add issue labels.

@quicksketch
Copy link
Member

I merged backdrop/backdrop#4424 into 1.x for 1.25.0. @leeksoup if you encounter any problems, please file a follow-up issue. Today is the last day before feature freeze for 1.25.0 so we wanted to get this in quickly. Thanks!

backdrop/backdrop@6860f7e by @klonos, @yorkshire-pudding, @stpaultim, @leeksoup, and @quicksketch.

@klonos klonos removed their assignment May 1, 2023
@leeksoup
Copy link
Author

leeksoup commented May 2, 2023

@quicksketch - No issues. Works great!

@jenlampton jenlampton changed the title Add mini pager option to Search module Add a mini pager option to Search module results. May 3, 2023
@klonos
Copy link
Member

klonos commented May 3, 2023

@quicksketch (or any of our other @backdrop/core-committers) tiny PR to remove a now irrelevant/innaccurate inline comment that we missed to remove from the changes that got merged in: backdrop/backdrop#4430

@quicksketch
Copy link
Member

Yup, clearly an accidental inclusion. I've merged it for 1.25.0. Thanks @klonos!

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