Skip to content

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

Merged
fzaninotto merged 23 commits intomasterfrom
ui_select_queries
Jun 28, 2015
Merged

[RFR] Send API request on reference auto-complete#508
fzaninotto merged 23 commits intomasterfrom
ui_select_queries

Conversation

@jpetitcolas
Copy link
Copy Markdown
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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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
Copy Markdown
Member

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@jeromemacias
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Great work, thanks a lot!

@endel
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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