dev/core#628 - Make Quicksearch options sortable #14441
Conversation
|
(Standard links)
|
| $element = $this->getElement('contact_autocomplete_options'); | ||
| $element->_elements[0]->_flagFrozen = TRUE; | ||
| $element->_elements[0]->setAttribute('disabled', 'disabled'); |
There was a problem hiding this comment.
ideally we would also specific required_options in metadata & handle this there (I'm OK with this being out of scope for this but possibly this whole form could be removed - like #14425 if we did move that to metadata
There was a problem hiding this comment.
I'm not sure that the current class structure is such a bad one. Makes sense to me to have all settings form inherit from a parent which does all the heavy lifting but small tweaks and overrides can be done in those classes.
These specific tweaks strike me as too weird to be generalizeable and I'm not entirely sure that "no form classes for settings pages" ought to be our goal, especially if it means "abstracting" every quirky setting behavior into a one-off metadata property.
| * @param $value | ||
| * @return bool | ||
| */ | ||
| public static function enableOptionOne(&$value) { |
There was a problem hiding this comment.
I feel like we need someone off the form layer for this - should apply to when it's set by api too
There was a problem hiding this comment.
Right, this is a callback set in the settings metadata. So prior to this PR it would only get handled correctly in the form layer. Now it will work everywhere.
There was a problem hiding this comment.
@colemanw right - but I think the location of the function is wrong - I'm not sure where it should go - maybe a new class for setting callbacks, maybe setting bao, maybe select values - but not on the form
There was a problem hiding this comment.
Yea I agree but could we take care of that in a diff PR. There are already other metadata-declared callbacks in this form class so they will all need to be relocated.
There was a problem hiding this comment.
I guess - but you are adding a callback here - still I'm OK for you to leave out of scope
|
Well I tested & it works & is an improvement - merging but ideally we can move that fn again! |
Overview
This is a reviewers cut of #13565, implementing the feature of allowing quicksearch options to be rearranged, but doing so in a way that is reusable for potentially other settings without any special handling.
See https://lab.civicrm.org/dev/core/issues/628
Before
After