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
JIP-20 Filter posts by location #17
Conversation
$near: { | ||
$geometry: { | ||
type: "Point", | ||
coordinates: [parseFloat(req.query.lon), parseFloat(req.query.lat)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we just pass in lat/lon not req.query and it is all converted
@@ -201,7 +201,7 @@ class DataGenerator { | |||
const rawLocations = fs.readFileSync(this.locationPath); | |||
this.mockLocations = JSON.parse(rawLocations).map((location) => ({ | |||
type: "Point", | |||
coordinates: [location.lat, location.lng], | |||
coordinates: [location.lng, location.lat], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wack literally everywhere else its lat first.
const [name, setName] = React.useState(location.name); | ||
|
||
if (initialLocation === undefined) { | ||
// TODO: this block is causing getPostsByFilter to be called twice when the page is refreshed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just keep lat/lon in the store rather than both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to but I had an issue with needing the map to update the post-list when I tried keeping it in the store 🐵
const [name, setName] = React.useState(location.name); | ||
|
||
if (initialLocation === undefined) { | ||
// TODO: this block is causing getPostsByFilter to be called twice when the page is refreshed | ||
// Another issue: user location is fetched here AND in post-slice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming this code block only needs to run once, to address these two issues maybe we could:
- Get rid of the
location
slice so the current location is only maintained in one place (i.e. in the post-slice). - Implement an action in the post-slice called
initLocation
that does sth like
navigator.geolocation.getCurrentPosition(function (position) {
state.locationFilter = {
lat: position.coords.latitude,
lon: position.coords.longitude,
name: undefined,
};
});
- Dispatch
initLocation
in auseEffect
hook inHomePage
orMap
(to avoid redundant calls/renders) - Or we could just call the above code block in a
useEffect
hook and set it in the store (no additional action necessary)
We could also keep the location-slice and just have all location-related store logic stay in there (which implies we remove location logic from post-slice)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it the second way and it fixes most of the problem
Just noticed that we also called getPostsByFilter for the initialLocation, but at least the duplicate requests for the detected location are gone 🐵 🐵
postState; | ||
const response = await axios.get( | ||
`${baseUrl}?` + | ||
`sort=${sortType}&` + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
axios does query params for you, just pass all of that inside an object with a params
field. https://github.com/axios/axios#example
You don't even have to worry about url encoding each component
params: { | ||
posts: [], | ||
sortType: postState.sortType, | ||
locationFilter: postState.locationFilter, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Not sure how I feel about objects in query params. We should prob just separate lat and lon.
longitude/latitude is out of bounds
and I guess that makes sense since mongo docs say longitude comes first)KNOWN ISSUES that aren't noticeable on the frontend but are bad if you think about them:
Probably going to log bugs for the issues to look at later but also open to suggestions