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

SearchKit: Don't show the pager unless it is needed #26395

Merged

Conversation

larssandergreen
Copy link
Contributor

@larssandergreen larssandergreen commented Jun 1, 2023

Overview

We don't need to show the pager in Search Displays if the number of rows is less than the page size.

Before

E.g. Manage Contribution Pages, pager just taking up space.
image

After

No more pager.
image

If you adjust the page size, the pager is shown as appropriate.

@civibot
Copy link

civibot bot commented Jun 1, 2023

(Standard links)

@civibot civibot bot added the master label Jun 1, 2023
@larssandergreen
Copy link
Contributor Author

jenkins test this please

1 similar comment
@larssandergreen
Copy link
Contributor Author

jenkins test this please

@aydun
Copy link
Contributor

aydun commented Jun 1, 2023

Looks good to me but let's see what @colemanw says

@colemanw
Copy link
Member

colemanw commented Jun 2, 2023

I don't entirely agree with this change. In general, when an element is not usable we display it as "disabled" rather than hiding it completely (search button, actions menu, unavailable actions). IMO that makes for a less-confusing UX. And I don't quite see the argument for "taking up space" being a problem, since the upshot of having fewer results than the limit is that the display will already take up less space than normal.
I would support making this a configurable option so the admin can choose this behavior if they wish. (and the configuration screen for table displays has grown quite cluttered so we probably need to start thinking about organizing it better).

@larssandergreen
Copy link
Contributor Author

Sure, we could do it as an option instead. I think it would be good to be able to have something like this for Admin UI, where in many cases there won't be a second page and we don't really need a disabled pager at the bottom of each. I agree that it doesn't make much difference for something like a contact search. Will do.

And agreed on the organization of the options.

@larssandergreen
Copy link
Contributor Author

@colemanw Are you happy if I add this option to the existing Admin UI displays?

@colemanw
Copy link
Member

colemanw commented Jun 2, 2023

@larssandergreen that sounds fine. We don't need to block progress just because the admin screen is cluttered, go ahead and add to it and we can try to de-clutter at some point.

@larssandergreen
Copy link
Contributor Author

@colemanw Will do, but I meant, are you happy to hide the pager when there is only one page for existing AdminUI Displays, like on Manage Contribution Pages in the example I gave. Looking through them, I think the only one that could be an edge case is Manage Groups, that one could go either way, the remainder I think really don't need the pager as most installs will probably never hit 50 of them.

@larssandergreen larssandergreen force-pushed the No-pager-in-Afform-when-not-needed branch from 82c3bbe to 62d8e67 Compare June 2, 2023 19:29
@colemanw
Copy link
Member

colemanw commented Jun 2, 2023

Ok @larssandergreen yes I'm fine with that.

@larssandergreen
Copy link
Contributor Author

OK, great, @colemanw, I've pushed two commits, one adds the hide_single option (and re-orders the pager options to make a little more sense), the other adds hide_single to all the Admin UI forms. Also added show_count and expose_limit to Groups and Scheduled Jobs for consistency.

@colemanw
Copy link
Member

colemanw commented Jun 4, 2023

Ok thanks @larssandergreen - looks great.

@colemanw colemanw merged commit 224d012 into civicrm:master Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants