-
-
Notifications
You must be signed in to change notification settings - Fork 824
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
Change custom contact ref groups selector to use select2 #12234
Conversation
@@ -237,6 +237,7 @@ public function setDefaultValues() { | |||
else { | |||
$defaults['is_active'] = 1; | |||
$defaults['option_type'] = 1; | |||
$defaults['is_search_range'] = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this default is a change in behaviour outside the main PR content & I'm not 100% sure everyone would agree.
It's more usable & less confusing but on a large database there is a real cost to creating a new searchable field.
I tested in production (Spark) and it's a nice improvement (this was reported as a bug by a Spark user). With regards to the 'is searchable' by default, I agree with Eileen. On the other hand, in 95% of the time, for new users, they probably want this option by default. Would a help text warning about performance be a compromise? Related docs page: https://docs.civicrm.org/user/en/latest/organising-your-data/creating-custom-fields/#is-this-field-searchable "While you may be tempted to mark every field as searchable, doing so may unnecessarily clutter the Advanced Search custom field panel, when in fact certain fields will probably never be used in that way. You may toggle this option on or off at any time, so do not be overly concerned about arriving at a final decision when you first define a custom field." |
In general my approach to things like this is_searchable is to pull them into their own PR so they don't hold up the main work & they get the visibility / discussion they need. I think this would be mergeable now without that line based on you testing |
This PR is not changing any defaults. |
OK then - let's merge based on @mlutfy testing |
Overview
Improves the "Custom Field" form to use Select2 for choosing a group.
Before
After
Notes
While I was mucking around on this form I also did a cleanup of the tpl to fix the whitespace and bring the js code up to standards.