Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixes #4679 - Enhances input/autocomplete GUI behavior. Displays html la... #5242

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

Srokap commented Mar 17, 2013

...bel instead of bare guid

@Srokap Srokap commented on the diff Mar 17, 2013

engine/lib/input.php
+ $options['callback'] = 'input_livesearch_data_callback';
+ $options['limit'] = $limit;
+ if (get_input('match_owner', false)) {
+ $options['owner_guid'] = $user->getGUID();
+ }
+ $entities = elgg_get_entities_from_relationship($options);
+ if ($entities) {
+ foreach ($entities as $row) {
+ list($entity, $result) = $row;
+ if (elgg_instanceof($entity, 'user')) {
+ $result['html'] = elgg_view('input/userpicker/item', array(
+ 'entity' => $entity,
+ 'input_name' => $input_name,
+ ));
+ //@todo remove this case (used in messages plugin)
+ if (!in_array('groups', $match_on)) {
@Srokap

Srokap Mar 17, 2013

Contributor

I'd gladly get rid of this hacky control, we should be fine with guid's as values when having nice autocomplete input.

@mrclay

mrclay Mar 23, 2013

Owner

Not sure what you mean here. The work looks good though.

I'd like to add an admin page to test a bunch of the input views based on this.

@Srokap

Srokap Mar 23, 2013

Contributor

We do this exception to sometimes get usernames as values instead of guids. We decide based on profile selected, not by param. I don't like such logic as it makes trouble to extend to more complex cases, where the condition above is no longer valid.

Contributor

Srokap commented Mar 17, 2013

Rebased to most recent master to pass Travis tests

Contributor

Srokap commented Mar 19, 2013

To test, output this:

echo elgg_view('input/autocomplete', array(
    'name' => 'test1',
    'match_on' => 'all'
));

echo elgg_view('input/autocomplete', array(
    'name' => 'test2',
    'match_on' => 'users'
));

echo elgg_view('input/autocomplete', array(
    'name' => 'test3',
    'match_on' => 'groups'
));

echo elgg_view('input/autocomplete', array(
    'name' => 'test4',
    'match_on' => 'friends'
));

And run in JS console:

$('.elgg-input-autocomplete').each(function(){console.log($(this).val())});
Owner

ewinslow commented Mar 19, 2013

Does this or did this ever support multiple selected values?

Contributor

Srokap commented Mar 19, 2013

AFAIK it didn't support multiple values and doesn't support now. I can add it though. The idea crossed my mind when working on it. We support multiple instances on one page now and that's new.

Owner

ewinslow commented Mar 19, 2013

I definitely like your improvements. We'll likely change it again for 1.10, but this is a great UX contribution.

Contributor

Srokap commented Mar 19, 2013

I'll add "multiple" support here before we merge it.

Owner

ewinslow commented Mar 19, 2013

If you have time. Not at all required.
On Mar 19, 2013 10:48 AM, "Paweł Sroka" notifications@github.com wrote:

I'll add "multiple" support here before we merge it.


Reply to this email directly or view it on GitHubhttps://github.com/Elgg/Elgg/pull/5242#issuecomment-15131307
.

Contributor

Srokap commented Mar 19, 2013

It's nice to have it in one push and it's not that much work for me.

Contributor

Srokap commented Mar 20, 2013

Just added multi value selection capability.

Owner

ewinslow commented Mar 20, 2013

Delete button is not showing up after I select an item. I can't undo my selection.

Can you also add an example to the theme_preview that demonstrates the "multiple" feature?

Contributor

Srokap commented Mar 20, 2013

Can you describe when don't you have delete button? I've tested it in detail for multiple selects case. Might have made mistake for single selection but rather unlikely. Please clear the cache first.

Example makes sense of course.

@mrclay mrclay and 1 other commented on an outdated diff Mar 23, 2013

engine/lib/input.php
@@ -457,6 +379,42 @@ function input_livesearch_page_handler($page) {
}
+/**
+ * @param stdClass $row
+ * @return array
+ * @access private
+ * @see input_livesearch_page_handler
+ */
+function input_livesearch_data_callback($row) {
@mrclay

mrclay Mar 23, 2013

Owner

prefix this with "elgg"

@Srokap

Srokap Mar 23, 2013

Contributor

Fixed

Contributor

Srokap commented Mar 23, 2013

I've added examples in theming sandbox miscellaneous section.

Contributor

Srokap commented Mar 26, 2013

@juho-jaakkola just pointed out similarity of this view to the input/userpicker. How did we came up with two separate views doing very similar thing? I don't remember any of them from 1.7. I think we should probably merge them to use common core and just changing parameters.

Owner

juho-jaakkola commented Mar 26, 2013

I think there should be two different input fields with autocomplete feature.

  1. One that just autocompletes the value of the input field but doesn't bind it to any specific entity. This field wouldn't require the value to exist. (Could be used for example to fill in the location field in user profile.)
  2. One that actually pics one or more entities so that the receiving action can then interact with those specific entities.
Contributor

Srokap commented Mar 26, 2013

For 1. we'd probably need to come up with smarter way of determining this value from autocomplete in /livesearch - something smarter than $entity->getDisplayName()

Owner

mrclay commented Mar 26, 2013

I don't have a lot of hope that these views can be made to cover most cases. Two cases we've needed in the past couple years: 1. A userpicker that returned groups as well as users, and selecting the group would load its members into the selection. 2. A userpicker that also returned non-users from an LDAP query, so the selection was a mix of users and LDAP usernames.

I guess my point is that site needs are likely to be too varied to be able to reuse these views all that much.

On the backend I'd like a small API to allow more easily constructing a handler like "livesaerch" and then allow most autocomplete views to specify that handler when sending the request (the userpicker has this option).

Contributor

Srokap commented Mar 26, 2013

From my experience, such view is extremally useful, however option to define customized search profiles is a must - see plugin hook I'm introducing here: https://github.com/Elgg/Elgg/pull/5242/files#L0R340

Main benefit is when creating pulldown to select entity, we might fetch too much items (ie. selection of related entities).

My idea is that:

  • input/autocomplete is most general view, pretty much looks like current version (+ easy switching query type would be useful)
  • we select as values, what is returned from handler (separate profiles for getting users names or guids)
  • we implement input/userpicker that uses input/autocomplete + some JS to switch profiles with checkbox
  • we register all possible profiles via plugin hook (very similar to current way)
  • we route everything via /livesearch page handler, don't support other ways - keep it simple
  • we might want to indicate to the entity view that we render it for autocomplete, but anyway it seems to run pretty nice already

This way we support common mechanism for defining any complex use cases and still keep several ones supported by the core. So actually I think we may help to cover most cases with minimal work.

Contributor

cash commented May 21, 2013

@brettp why do we have userpicker and autocomplete views?

I think the answer is that userpicker lets you select multiple users, gives you a visual representation, and may have been intended to replace the friendspicker. Autocomplete was meant to be more general purpose, only select one value, and wasn't intended to have any fancy graphics but do autocomplete in a text box.

Contributor

cash commented Jun 27, 2013

I really like this. Thanks for putting together the demo page in the theme sandbox. It made it very easy to test.

Only issues I noticed:

  • the confirm on the remove button was really annoying. I don't think it needs a confirm since you can easily search for the user again
  • the group display includes its full entity menu (even both a like and unlike button).
  • it lets a user add the same user/group multiple times
  • how about a few unit tests for search function?

It does look like we can deprecate the userpicker because of the improvements to this view.

Contributor

cash commented Jul 8, 2013

@Srokap Do you think you will be able to work on this before the 1.9 release?

@ghost ghost assigned Srokap Jul 11, 2013

Contributor

cash commented Jul 13, 2013

I've rebased this. Will push later today.

Owner

ewinslow commented Aug 10, 2013

@cash or @Srokap please reopen when the code is ready to be reviewed again.

@ewinslow ewinslow closed this Aug 10, 2013

@Srokap Srokap referenced this pull request May 15, 2014

Closed

fix(livesearch): removed custom queries with ege* functions #6792

1 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment