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

Custom marker #22

Closed

Conversation

nrslt
Copy link
Contributor

@nrslt nrslt commented Nov 24, 2020

Hi everybody,

This PR was done to change the way we instantiate the sites and alerts layers: from a dl.GeoJSON layer to a list of dl.Marker objects grouped in a dl.MarkerClusterGroup (for sites) or a dl.LayerGroup (for alerts) layers.
Such changes imply modifying some functions as well as several callbacks, as described below.

Small changes

Along the way, small changes/renaming were performed on some functions or components id:

  • the departments choice component above the map has been removed since it was not used
  • the function build_alerts_geojson() in alerts.py has been changed to build_departments_geojson() since it concerns departments
  • the build_alerts_map() function from utils.py has been renamed to build_info_box() in order to differentiate it from the ‘real’ build_alerts_map()function from alerts.py)

About markers layers

Sites markers

  • The functions get_cameras_position() and build_cameras_markers() in alerts.py were merged → build_sites_markers(). At the moment no API call is performed and we still read a .csv file.

  • One major change from the precedent state of the app is that the dl.MarkerClusterGroup feature was used instead of a more 'classic' layer such as dl.LayerGroup. This way, we get to cluster the markers when zooming out, which makes the map and the information it contains easier to visualize.

Alerts markers

  • A new function build_alerts_markers() was created → custom markers were added as a list of dl.Markers objects, grouped inside a dl.LayerGroup layer before being added to the dl.Map object

  • Each dl.Marker object has been instantiated with an id as well as dl.Tooltip and dl.Popup features

Screenshot 2020-11-25 at 11 01 39

Callbacks

Note that several callbacks have been changed: the idea is to make the most of dl.Marker objects features (tooltip and popup) before relying on callbacks. This can definitely be debated.

Still to be done

dealing with popup video and the html.Button component in order to display an image on the left panel of the app
at the moment, this feature only works fine for one particular alert (number 2), though I still don't know why it behaves that way and does not work for other alerts markers. I'll investigate this issue today.

nrslt and others added 12 commits October 12, 2020 13:00
getting up to date with original repo
feat: Set up dockerized server for the dash app (#3)
…jects to be used in the homepage.py file ; changed the layout of the homepage in order to better fit our needs
feat: Sets the project for automatic deployment on Heroku (#5)
graphs.py new file + some changes in homepage.py layout ! (#7)
Added the slider and the callback for interactivity (#8)
@Akilditu
Copy link
Collaborator

@nrslt Thanks for the great PR !

I do think that we could submit the black tower-ish markers to vote before merging haha 😄

Will have a deeper look by tomorrow !

@nrslt
Copy link
Contributor Author

nrslt commented Nov 24, 2020

@Akilditu unfortunately the tower is a non-negotiable feature...
😄

Copy link
Collaborator

@Akilditu Akilditu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dealing with popup video and the html.Button component in order to display an image on the left panel of the app

Will adress this in the next PR

markers = dlx.geojson_to_geobuf(markers)
return markers
# We group all dl.Marker objects in a dl.LayerGroup object
markers_layer = dl.MarkerClusterGroup(children=markers, id='sites_markers')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this ClusterGroup creates the green clustering marker with the "2" value here ?

If yes did you do that on purpose for better UI when map is zoomed ?

Copy link
Contributor Author

@nrslt nrslt Nov 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly, yes

when discussing this issue with @VincGargasson, we came to the conclusion that it became tricky to use callbacks to reproduce the behaviour: "click on a department" --> "the corresponding department's cameras/sites appear"

At this point we imagine that the filtering on a particular department's cameras/sites will be done when the user connects to the app. Maybe we add that filter inside the `build_sites_markers' function directly as an argument.

Furthermore, when you add markers, they do not resize themselves when zooming out, so the ClusterGroup layer seemed like a nice compromise in order to not overload the map. It is completely open to debate, though!

@Akilditu
Copy link
Collaborator

@Akilditu unfortunately the tower is a non-negotiable feature...
😄

@pechouc, @abdi-adel what do you think ? 😄

@nrslt
Copy link
Contributor Author

nrslt commented Nov 25, 2020

@Akilditu unfortunately the tower is a non-negotiable feature...
😄

@pechouc, @abdi-adel what do you think ? 😄

I'm just kidding haha

I think ideally a good marker will have a sort of arrow/tick that enables to locate the exact location quickly by pointing toward it (such as the default marker you'll tell me...) so I put the black tower but I agree that it's a bit too much 😄

@VincGargasson
Copy link
Collaborator

Great job @nrslt ! The button to show the video is a good idea for now. Maybe we won't need anymore a button as the fireman need to see the picture each time a popup shows up.
Just asked 2 questions in your script

@VincGargasson VincGargasson deleted the branch pyronear:vincent-adding-popup-to-marker December 6, 2020 11:35
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.

None yet

3 participants