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

Add "searchMode" option for Feature Layer Provider #251

Merged
merged 12 commits into from
May 26, 2020

Conversation

pmacMaps
Copy link
Contributor

@pmacMaps pmacMaps commented May 22, 2020

Possible values for "searchMode":

  • "contain" = wildcard
  • "startWith" = starting text and then wildcard after
  • "endWith" = wildcard then text
  • "strict" = entered text must match exactly

@gavinr
Copy link
Contributor

gavinr commented May 22, 2020

Thanks for all your work on this @pmacMaps. Really appreciate it. I tested out the functionality and it seems to work as expected. Added functionality like this calls for unit tests, so we should tack on some unit tests to this PR. I understand that you've done a lot of work on this already, so if you'd like to add those, we would appreciate it but if not, I'll try to find some time to add them.

@pmacMaps
Copy link
Contributor Author

I can take a stab at the unit tests. I've never made any before, so any links or guidance is appreciated.

I know we'll also need to update the API reference.

@gavinr
Copy link
Contributor

gavinr commented May 22, 2020

@pmacMaps I believe the additional unit tests will go in https://github.com/Esri/esri-leaflet-geocoder/blob/master/spec/Providers/FeatureLayerSpec.js - right now it has a beforeEach which runs on each test, I think we'll have to change that so that you have a separate "section" of tests that will create a privder that the new searchMode parameter and then runs a few tests to make sure the url that is used is correct.

we are using karma and mocha, but you'll mostly want to focus on the mocha documentation.

@pmacMaps
Copy link
Contributor Author

The unit tests may get written faster if someone with experience tackles the issue. I can definitely update the API reference once this functionality ships.

@jgravois
Copy link
Contributor

jgravois commented May 24, 2020

@pmacMaps good on you for jumping in to make this contribution!

The unit tests may get written faster if someone with experience tackles the issue.

no pressure, but i can say from personal experience that i was initially extremely intimidated by the idea of writing unit tests and that the intimidation gave way to immense gratification when i finally stuck my toe in the water.

if you're interested, you can learn heaps by making tiny changes to existing tests one at a time to get practical experience breaking and fixing them. to this day, when i write new tests i still usually rely heavily on existing tests in the suite and just follow the patterns when writing new tests without worrying about understanding exactly how they work.

for you the existing test below would be a great one to copy/paste and alter slightly:

it('should correctly build the searchExtent for the provider', function (done) {
var geosearch = L.esri.Geocoding.geosearch({
providers: [
L.esri.Geocoding.arcgisOnlineProvider({
maxResults: 6
})
],
searchBounds: mapbounds
}).addTo(map);
geosearch._geosearchCore._suggest('Mayoworth, WY');
var request = geosearch._geosearchCore._pendingSuggestions[0];
expect(request).to.be.an.instanceof(XMLHttpRequest);
this.requests[0].respond(200, { 'Content-Type': 'application/json' },
JSON.stringify({'suggestions': []}));
expect(geosearch._geosearchCore._pendingSuggestions[0].url).to.equal('https://geocode.arcgis.com/arcgis/rest/services/World/GeocodeServer/suggest?text=Mayoworth%2C%20WY&location=-97.64%2C30.32&distance=50000&searchExtent=%7B%22xmin%22%3A-101.78%2C%22ymin%22%3A28.28%2C%22xmax%22%3A-93.5%2C%22ymax%22%3A32.36%2C%22spatialReference%22%3A%7B%22wkid%22%3A4326%7D%7D&maxSuggestions=6&f=json');
done();
});

you'd just need to ensure your own test instantiated a provider using the new option you added and make sure it tests for the corresponding text={value}.

@gavinr gavinr merged commit 581d689 into Esri:master May 26, 2020
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.

FeatureLayerProvider: return suggestions that start with the input string
5 participants