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

dev/core#2187 Allow sites to customise the default number of rows ret… #18969

Merged
merged 1 commit into from
May 11, 2021

Conversation

seamuslee001
Copy link
Contributor

…urned from QuickForm Searches

Overview

This allows sites to customise the default number of rows returned by a QuickForm search rather than it being hard coded to 50 and users having to manipulate the counter at the bottom on each search.

Before

Default number hard coded as a constant 50 in CRM_Utils_Pager

After

Site configurable setting for number of search results returned defaults to 50 which was the original constant value.

Technical Details

I left the constant in CRM_Utils_Pager in case extensions had copied across code from core that used it but separated it out from the others to show it was deprecated.

ping @mattwire @eileenmcnaughton @MikeyMJCO @JoeMurray

You folks might be interested in this

@civibot
Copy link

civibot bot commented Nov 13, 2020

(Standard links)

@civibot civibot bot added the master label Nov 13, 2020
@agileware-justin
Copy link
Contributor

Related Gitlab, https://lab.civicrm.org/dev/core/-/issues/2187

@eileenmcnaughton
Copy link
Contributor

So we have

  • quickform searches
  • reports
  • search kit

This is only for the first from the looks - unless we intend to use it on the others we should make the setting more specific (e.g append quick_form)

@colemanw

@seamuslee001
Copy link
Contributor Author

I would be happy to extend it further but figure start at Quickform,

@eileenmcnaughton
Copy link
Contributor

Yeah - I don't know if it makes sense to - ie current default for reports is 50 & for searchkit 10? I'll see what @colemanw thinks

@demeritcowboy
Copy link
Contributor

This seems to have stalled since November and it's not clear if there's agreement about the concept. Is there still interest in this?

@homotechsual
Copy link
Contributor

I think this is a good enhancement.

@colemanw
Copy link
Member

I think it would be fine for SK to use this new setting.
👍 on this PR from me.

'is_domain' => 1,
'is_contact' => 0,
'description' => ts('What is the default number of records to show on a search'),
'help_text' => ts('Set this to show a specific number of records'),
Copy link
Contributor

Choose a reason for hiding this comment

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

There's an open ticket about this somewhere that help_text doesn't work in the "new" settings system and it causes an error: User warning: Smarty error: unable to read resource: "CRM\Admin\Form\Setting\Search.hlp" in Smarty->trigger_error() (line 1108 of ...\packages\Smarty\Smarty.class.php). Until that's fixed you'll need to either leave out the help_text here or put some smarty in the tpl. The description here seems adequate so I think can leave it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok removed

'maxlength' => 3,
],
'default' => 50,
'add' => '5.33',
Copy link
Contributor

Choose a reason for hiding this comment

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

It's no longer 5.33.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated now

@colemanw
Copy link
Member

Ok I think we're good to go here.

@seamuslee001 seamuslee001 merged commit cac2bad into civicrm:master May 11, 2021
@seamuslee001 seamuslee001 deleted the dev_core_2187 branch May 11, 2021 23:08
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