Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat : Added new markers types and live alerts features #23
feat : Added new markers types and live alerts features #23
Changes from 25 commits
b6ffde7
f5f2a2a
a3fc215
16fb89e
d0b8714
4bb3088
1ed7ff7
b46ad7d
9754c40
1d593b1
f45f07e
2dd12df
3d53902
3213a60
c50eff2
f7d81ee
d63151f
39c95c3
4d4b94a
abc8e4a
f7feada
9d0fb67
11a710d
2a06a3c
9dc1705
e650cce
a73ad41
fbd6308
57293d5
cf39f68
e957a68
2e826e1
ae53eee
d814ba3
64e46dc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Thanks a lot for the renaming from
build_alerts_geojson
tobuild_departments_geojson
or fromget_camera_positions
tobuild_sites_markers
, it makes the code much clearer! In the same vein, shouldn't we changecamera_positions
intosite_positions
?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.
Small typo in the comment.
Just to confirm and if I am not mistaken, the
site
table that we will call via the API contains asite_id
which we will be able to use instead of the index of the for loop here, right?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.
yes I think you're right @pechouc, the f-string id was the previous way of getting a marker, from now on we should have a real site_id
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.
Probably not a priority for the moment but I wonder whether it is a good practice to define an extensive set of variables which we use only once, a few lines below (like
lat
,lon
,site_name
...). But clearly, we can move forward with this until demo day, as variable names are explicit.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 guess this has changed since the comment was written down!
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.
Maybe we can call it
markers_cluster
instead ofmarkers_layer
as the type of object seems to have changed?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.
To make it more explicit in case we want to call that id in a call-back at some point?
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.
Amazingly low value-added comment 😅
But so far, for sites and alert markers, we always add an underscore before the id.
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 really like the fact that everything is already variabilized outside of the
build_live_alerts_metadata
function 😄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.
Isn't this part a duplicate of what is inside the
Homepage
function? Sorry if I am missing something obvious, but is there any reason we should keep it?