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

Flexible Element Search #7775

Merged
merged 3 commits into from
Jul 12, 2020
Merged

Flexible Element Search #7775

merged 3 commits into from
Jul 12, 2020

Conversation

blackboxlogic
Copy link
Contributor

Fix #7627
Accept a variety of formats in Feature Search when specifying an element by type and id.
For example: n123, n 123, node123, node 123 or even https://www.openstreetmap.org/node/123/history#map=19/43.66140/-70.25415

Accept a variety of formats in Feature Search when specifying an element by type and id.
For example: n123, n 123, node123, node 123 or even https://www.openstreetmap.org/node/123/history#map=19/43.66140/-70.25415
@blackboxlogic
Copy link
Contributor Author

I ran npm test and got errors that didn't look related to my changes.

modules/ui/feature_list.js Outdated Show resolved Hide resolved
modules/ui/feature_list.js Outdated Show resolved Hide resolved
modules/ui/feature_list.js Outdated Show resolved Hide resolved
@kymckay
Copy link
Collaborator

kymckay commented Jul 10, 2020

We can add some tests for all the cases we want to match/don't match too so that this improved functionality never gets accidentally broken 👍

Don't worry about that in this PR though, I'd be happy to do that kind of maintenance after merge

@blackboxlogic
Copy link
Contributor Author

blackboxlogic commented Jul 10, 2020

I wanted to add tests, but didn't know where to put them, or how best to extract the function for testing. Thanks for handling it.
Positive Cases:
n1, N1, n01, n11, w1, r1, node1, way1, relation1, n 1, node 1, ?n1, n?1, n1?, ?n?1, ?n1?, n?1?, ?n?1?, 1?n?1?n, n?1?1, n?n?1
www.example.com/node/1/history?lat=100&lon=100
Negative Cases:
nn1, n1n, 1n1, 1n, n??1, n0, n, 1, [empty string]

@quincylvania
Copy link
Collaborator

@blackboxlogic Thanks for working on this! Welcome to the dev side of iD 🔨

@SilentSpike Thanks a bunch for taking an interest. The code looks reasonable to me at first glance, but I'll defer to you. Feel free to merge and tweak this as needed. (Tests are always stellar too, since you mentioned them.)

Oh, and if anyone's feeling inspired it might be a good idea to break out some of this functionality into separate functions instead of having everything in features().

@kymckay
Copy link
Collaborator

kymckay commented Jul 12, 2020

@blackboxlogic I've tested this and it's good to go, will merge as is now 👍

If you'd like to learn how to add the tests yourself, let me know and I an point you in the right directions 😃 If not, I don't mind taking care of it.

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.

More flexible format in 'Search Features' when searching by element id
3 participants