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

[RFR] Send API request on reference auto-complete #508

Merged
merged 23 commits into from
Jun 28, 2015
Merged

Conversation

jpetitcolas
Copy link
Contributor

Instead of having a pre-define set of choices in reference fields, send API requests at will to fetch related records.

anim

Left tasks:

  • Implement API auto-complete on single reference fields (post id)
  • Display correctly selected value
  • Implement API auto-complete on multiple reference fields (tags)
  • Implement tests
  • If no fields configured, disable refresh and populate with all data (for BC)
  • Update admin-config repository to deal with refreshDelay
  • Update documentation
  • Update blog sample configuration
  • Test it on real world project


return ReadQueries.getAllReferencedData(referenceFields, search)
.then(r => r[field.name()])
.then((results) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need parentheses for a single argument

@jpetitcolas
Copy link
Contributor Author

Switching to RFR. I still need to test it on a real world project before merging.

@jpetitcolas jpetitcolas changed the title [WIP] Send API request on reference auto-complete [RFR][DO NOT MERGE] Send API request on reference auto-complete Jun 18, 2015
@jpetitcolas jpetitcolas changed the title [RFR][DO NOT MERGE] Send API request on reference auto-complete [RFR] Send API request on reference auto-complete Jun 19, 2015
@jpetitcolas
Copy link
Contributor Author

Tested on real world project. Seems to work. Removing the [DO NOT MERGE].

}
scope.getChoices = typeof(choices) === 'function' ? choices : function() { return choices; };
var choices = field.choices();
scope.choices = typeof(choices) === 'function' ? choices(scope.entry) : choices;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this works, I had to return a function because Angular complained about an infinite look. Needs further testing.

@fzaninotto
Copy link
Member

If you have to put some API fetching logic in a directive, please move it to a service.

expect(uiSelect).toBeTruthy();
});

it('should call remote API when inputting first characters', function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something bothers me: users can't opt out for ajax autocompletion. That means that if their API doesn't implement the search query param, reference fields cease to work. This is a serious BC break.

Autocomplete should be opt-in ; the default behavior should still be to fetch all possible values, and only autocomplete locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this is already dealt through the field.refreshDelay parameter. If it's null, then all choices would be retrieved, the same as before my changes.

I put the API request behavior by default, as it sounds more logical to me for references. Indeed, user doesn't know how many records there is remotely. And it is more probable that there is a lot. It would then be limited by the perPage parameter.

This is indeed a BC break, but it improves the component behavior IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As seen with @fzaninotto, we'll set refreshDelay to null by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, you must make it more explicit that in the doc that a Reference is populated by the entire list of references, and that if the developer wants to use autocomplete, they should set the refreshdelay to something more than null.

@jpetitcolas
Copy link
Contributor Author

Refresh logic moved to a dedicated service. Ready for another review.

@jeromemacias
Copy link
Contributor

Is it possible to disable first request (in routing) for retrieving references values in case of refreshDelay is not null ?
Something like view.getReferencesWithNoRefreshDelay().

@jpetitcolas jpetitcolas changed the title [RFR] Send API request on reference auto-complete [WIP] Send API request on reference auto-complete Jun 24, 2015
@jpetitcolas
Copy link
Contributor Author

Back to WIP for optimizing number of sent requests and to fix autocomplete, which seems to be made only on local records.

@jpetitcolas
Copy link
Contributor Author

Ready for review.

Failing tests are due to the admin-config PR.

@jpetitcolas jpetitcolas changed the title [WIP] Send API request on reference auto-complete [RFR] Send API request on reference auto-complete Jun 26, 2015
@jeromemacias
Copy link
Contributor

👍

fzaninotto added a commit that referenced this pull request Jun 28, 2015
[RFR] Send API request on reference auto-complete
@fzaninotto fzaninotto merged commit 2d13d2d into master Jun 28, 2015
@fzaninotto
Copy link
Member

Great work, thanks a lot!

@endel
Copy link
Contributor

endel commented Jul 25, 2015

Hey guys!

Is there any way to get the Field reference inside the filters function?
It seems that the scope gets complete lost when the function is called.

Check out this example:

...
let field = nga.field(singular + "_id", 'reference').
  targetEntity(entities[plural]).
  targetField(nga.field(label_field)).
  filters(function(search) {
    console.log(field) // <= Uncaught ReferenceError: field is not defined
  }).
  remoteComplete(true, { refreshDelay: 300 }).
  singleApiCall(aggregateIds);

What could I do?

@fzaninotto
Copy link
Member

@endel: not for now. But this reference.filters() method needs an overhaul anyway.

@jeromemacias jeromemacias deleted the ui_select_queries branch July 28, 2015 08:58
@fzaninotto
Copy link
Member

reference.filters() has just been renamed to reference.permanentFilters() (refs #588).

@endel I can't reproduce your issue. Could it be due to babel translation? What if you use a var instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants