Skip to content

Conversation

@esirK
Copy link
Contributor

@esirK esirK commented Feb 3, 2021

Description

Allows Filtering of nodes using location city

Fixes # (issue)

Type of change

  • New feature (non-breaking change which adds functionality)

Screenshots

Screenshot 2021-02-03 at 15 17 53

Screenshot 2021-02-03 at 15 18 03

Data
Screenshot 2021-02-03 at 16 33 47

Screenshot 2021-02-03 at 16 33 58

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

@esirK esirK requested a review from kilemensi February 3, 2021 12:19
@esirK esirK self-assigned this Feb 3, 2021
@esirK esirK requested a review from KhadijaMahanga February 3, 2021 12:19
@esirK esirK marked this pull request as draft February 3, 2021 12:19
@kilemensi
Copy link
Member

QQ @esirK , can we may pull all the nodes and check:

  1. They have cities, and
  2. If we need to do any clean up to make sure names match, etc.

@esirK
Copy link
Contributor Author

esirK commented Feb 3, 2021

QQ @esirK , can we may pull all the nodes and check:

  1. They have cities, and
  2. If we need to do any clean up to make sure names match, etc.

@kilemensi not all Nodes have cities

@kilemensi
Copy link
Member

QQ @esirK , can we may pull all the nodes and check:

  1. They have cities, and
  2. If we need to do any clean up to make sure names match, etc.

@kilemensi not all Nodes have cities

Do they have lat/long that we can use to reverse geocode @esirK ?

@esirK
Copy link
Contributor Author

esirK commented Feb 3, 2021

QQ @esirK , can we may pull all the nodes and check:

  1. They have cities, and
  2. If we need to do any clean up to make sure names match, etc.

@kilemensi not all Nodes have cities

Do they have lat/long that we can use to reverse geocode @esirK ?

I haven't seen any that's missing lat/long so yes we can do reverse geocode

@KhadijaMahanga
Copy link
Contributor

QQ @esirK , can we may pull all the nodes and check:

  1. They have cities, and
  2. If we need to do any clean up to make sure names match, etc.

@kilemensi not all Nodes have cities

Do they have lat/long that we can use to reverse geocode @esirK ?

I haven't seen any that's missing lat/long so yes we can do reverse geocode

@esirK they are a few with None

@esirK
Copy link
Contributor Author

esirK commented Feb 3, 2021

QQ @esirK , can we may pull all the nodes and check:

  1. They have cities, and
  2. If we need to do any clean up to make sure names match, etc.

@kilemensi not all Nodes have cities

Do they have lat/long that we can use to reverse geocode @esirK ?

I haven't seen any that's missing lat/long so yes we can do reverse geocode

@esirK they are a few with None

In such a case, I think we will need to manually add the city.

@kilemensi
Copy link
Member

@esirK / @KhadijaMahanga how do we even show a node on the map then?

@KhadijaMahanga
Copy link
Contributor

@kilemensi @esirK my bad, I think they all have lat/long...the None locations don't have nodes assigned to them

@esirK esirK marked this pull request as ready for review February 3, 2021 12:55
@esirK
Copy link
Contributor Author

esirK commented Feb 3, 2021

@kilemensi @KhadijaMahanga I think we can merge this as I work on the reverse geocode.

@KhadijaMahanga
Copy link
Contributor

Shouldn't we be filtering using lat & long instead?

@esirK
Copy link
Contributor Author

esirK commented Feb 3, 2021

Shouldn't we be filtering using lat & long instead?

I think we want something like v1/node?location__city=Nairobi the same way we are doing for the country

@KhadijaMahanga KhadijaMahanga self-requested a review February 3, 2021 14:17
@esirK esirK merged commit df29e0b into master Feb 3, 2021
@esirK esirK deleted the ft-allow-filtering-using-location-city branch February 3, 2021 14:22
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