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

Change custom contact ref groups selector to use select2 #12234

Merged
merged 2 commits into from
May 30, 2018

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented May 29, 2018

Overview

Improves the "Custom Field" form to use Select2 for choosing a group.

Before

screenshot from 2018-05-29 17 25 39

After

screenshot from 2018-05-29 17 25 04

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.

@@ -237,6 +237,7 @@ public function setDefaultValues() {
else {
$defaults['is_active'] = 1;
$defaults['option_type'] = 1;
$defaults['is_search_range'] = 1;
Copy link
Contributor

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.

@mlutfy
Copy link
Member

mlutfy commented May 30, 2018

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."

@eileenmcnaughton
Copy link
Contributor

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

@colemanw
Copy link
Member Author

This PR is not changing any defaults. is_search_range is a subsetting of is_searchable and was being set true by javascript, which was silly.

@eileenmcnaughton
Copy link
Contributor

OK then - let's merge based on @mlutfy testing

@eileenmcnaughton eileenmcnaughton merged commit ea2cc5c into civicrm:master May 30, 2018
@eileenmcnaughton eileenmcnaughton deleted the CustomForm branch May 30, 2018 20:14
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.

4 participants