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

Feature find task #287

Merged
merged 9 commits into from Jul 20, 2014
Merged

Feature find task #287

merged 9 commits into from Jul 20, 2014

Conversation

ghost
Copy link

@ghost ghost commented Jul 15, 2014

These commits are in the Public Domain. 🎉 #280

return this;
},

searchFields: function (searchFields) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about renaming this fields to better match whats in the query the identify tasks?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I was wondering about this. I agree we should keep the same naming convention across all the Tasks. I'll update the PR.

On the other note, what to you think about updating fields in Query/Identify to accept array or string?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed on making fields in Query/Identify accepting array or string.

@patrickarlt
Copy link
Contributor

Its probably also worth adding simplify to the find task since the API supports maxAllowableOffset https://github.com/Esri/esri-leaflet/blob/master/src/Tasks/Query.js#L66-L70

@robertd
Copy link
Contributor

robertd commented Jul 17, 2014

+1 on simplify... it makes sense.

robertd and others added 6 commits July 17, 2014 12:38
* Update MapService docs
* Expose find task from DynamicMapLayer
* Update DynamicMapLayer docs
* Add example page for Finding Features under Dynamic Map Layer section
* Fix few typos in docs
* Add Find task specs
* Update Find task API Docs with method name changes
* Add returnGeometry function to Query task and update the API docs
* Add simplify method to Find task and update API docs
* Update examples and fix few typos
* Query fields method can now accept arrays and strings
@robertd
Copy link
Contributor

robertd commented Jul 17, 2014

Updated the PR and rebased on current master (for easier merge).

@@ -10,6 +10,7 @@ L.esri.Tasks.Query = L.Class.extend({
}

this._params = {
returnGeometry: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what broke all the Sinon tests. By adding an extra param the request url changed and wasn't changed in the tests to reflect this. I made a PR that fixes all this https://github.com/rdjurasaj-usgs/esri-leaflet/pull/1

@robertd
Copy link
Contributor

robertd commented Jul 20, 2014

I'm curious.... Are all the tests passing for you now or you're still getting the infamous Error: timeout of 2000ms exceeded and Fake XHR onreadystatechange handler threw exception: expected null to deeply equal { Object (type, features) } as mentioned in #271?

@patrickarlt
Copy link
Contributor

All tests are passing. I think the errors in #271 are a result of some thing else. The tests have always passed locally on my machine. Travis has some other issues that need to be worked through thought.

Merging this since everything checks out. Thanks @robertd!

@patrickarlt patrickarlt reopened this Jul 20, 2014
patrickarlt added a commit that referenced this pull request Jul 20, 2014
@patrickarlt patrickarlt merged commit 2a28ddc into Esri:master Jul 20, 2014
@robertd
Copy link
Contributor

robertd commented Jul 20, 2014

👍

I hope to see one day tests passing on my computer. :) I'm officially giving up on sinon —
Sent from Mailbox

On Sun, Jul 20, 2014 at 4:55 PM, Patrick Arlt [email protected]
wrote:

Merged #287.

Reply to this email directly or view it on GitHub:
#287 (comment)

@ghost ghost deleted the feature-find-task branch July 21, 2014 16:20
jgravois pushed a commit to jgravois/esri-leaflet that referenced this pull request Apr 23, 2022
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.

2 participants