Use APIv4-based Autocomplete widget throughout SearchKit, Afform & API Explorer #24974
Conversation
|
(Standard links)
|
|
FYI: I have started a code read through review but not posted as I have some reservations on introducing new code using old javacsript libraries which have long since been supported by native JS. https://chat.civicrm.org/civicrm/pl/mhnp3ryc4ifrx8iybccwqhf3kw In terms of
I'm sure there's other places and tests, but I've run out of time. |
|
@colemanw see possible regression from @artfulrobot |
| staticItems = getStaticOptions(select2Options.static), | ||
| multiple = !!select2Options.multiple; | ||
|
|
||
| $el.crmSelect2(_.extend({ |
There was a problem hiding this comment.
Consider standard Object.assign call instead? See discussion
| if (!idsNeeded.length) { | ||
| callback(multiple ? existing : existing[0]); | ||
| } else { | ||
| var params = $.extend({}, apiParams || {}, {ids: idsNeeded}); |
There was a problem hiding this comment.
Consider standard Object.assign call instead? See discussion
Or at least comment as to why jQuery's implementation (and not Lodash's or vanilla's) is chosen here.
| @@ -31,7 +30,7 @@ class AfformAutocompleteSubscriber extends AutoService implements EventSubscribe | |||
| */ | |||
| public static function getSubscribedEvents() { | |||
| return [ | |||
| 'civi.api.prepare' => ['onApiPrepare', Events::W_MIDDLE], | |||
| 'civi.api.prepare' => ['onApiPrepare', -20], | |||
There was a problem hiding this comment.
Can we have a comment to explain the hard coded integer? W_MIDDLE at least gives a clue.
| return _.transform(staticItems || [], function(staticItems, option) { | ||
| staticItems.push(_.isString(option) ? staticPresets[option] : option); | ||
| }); |
There was a problem hiding this comment.
I find this hard to reason about.
staticPresets is a hard coded object.
We're _.transform-ing staticItems which, from a quick scan seems to be an Object, into a new copy of staticItems (or an array? - change of type?). It's confusing because: _.transform() which only iterates string keyed properties anyway, so why the _.isString() call which would always return true?
I guess it's saying: if one of the staticPresets keys is specified as a string in the staticItems array, replace it with the object above.
Is this the same - for this use case - as simply:
| return _.transform(staticItems || [], function(staticItems, option) { | |
| staticItems.push(_.isString(option) ? staticPresets[option] : option); | |
| }); | |
| return staticItems.map(option => staticPresets[option] || option) |
I think this is a lot easier to read/understand.
There was a problem hiding this comment.
It's actually iterating an array. The array contains either strings or objects. If it contains a string it's replaced with the matching object from staticPresets.
Maybe I should add a comment...
There was a problem hiding this comment.
@colemanw Yes, and my suggestion results in the exact same behaviour (I tested).
|
I'm still actively working on improving the output of the new widgets for parity (or better) with the old widgets. I've got another PR for that I'll post soon, and I'll include search-by-id as a default. |
|
Merging per discussion https://chat.civicrm.org/civicrm/pl/ch49u7k57b8emxrptmyzircb6c because @colemanw will provide follow-up PRs to address issues. |
|
@colemanw Please follow up with a PR to add comments where @artfulrobot suggests |
|
@mattwire @artfulrobot here is the followup, it would be great to get it reviewed soon :) |
|
@colemanw did you mean to include a link? |
|
@artfulrobot I was counting on you to telepathically know I was referring to #24976 |
|
@colemanw ah, I was getting the 2 and the 4 but it was a bit blurry after that 😆 |
Overview
This switches our 3 APIv4-based UIs to consistently use the new Autocomplete widget instead of the old v3-based entityRef
Before
v3 widget on v4 screens.
After
v4 only
Comments
You won't notice much difference in the UI as the widgets look the same. There are some differences in how the results are formatted as some v3 apis have customizations to give more verbose output. I'm working on recreating that in APIv4...