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

Added the slider and the callback for interactivity #8

Merged
merged 4 commits into from
Oct 21, 2020

Conversation

pechouc
Copy link
Collaborator

@pechouc pechouc commented Oct 20, 2020

Aim of the Pull Request

The goal of this PR is to add the slider that enables users to modify the opacity of department colours in the "Niveaux de Risque" dashboard. Since it is a relatively small feature, I thought that we could directly hold the conversation about how to improve it directly in this pull request.

Questions

The slider currently looks like:

Capture d’écran 2020-10-20 à 21 37 46

  • What do you think of the format of the slider? NB - as it might not be clear on the picture: marks are just indicative and can be clicked directly by the user, but any value between 0 and 1 can be chosen with a step of 0.01.

  • Would you like me to add it to the homepage or should we keep it only in the "Niveaux de Risque" dashboard for now?

  • As you will see, for the callback, I could not find a "lighter" solution than re-instantiating the colorbar and the geojson object. I have tried to find another way as I suspect this is a disproportionately heavy operation regarding the importance of the feature itself but nothing else worked. At least, it seems to work fine locally and doesn't break other callbacks but if you have any other idea, I would be glad to try them out!

Would you want some context about the Dash component used to create the slider, this page is very detailed about it.

Details of changes in the files

main.py

  • Added a dash_leaflet import, used when changing the children of the map object in the new callback. Same goes for the import of build_risks_geojson_and_colorbar - used to re-instantiate the colorbar and the GeoJSON as evoked previously - and build_info_object from other Python files.

  • Added the callback related to the slider, allowing users to choose the level of colour opacity they want to apply to the map. The way it works is not really satisfactory as the change it implies seems slightly disproportionate to me but I could not find any other solution.

risks.py

  • Added a dash_core_components import, necessary for instantiating the slider.

  • Moved the creation of the department object, which is based on the GeoJSON local file for now, outside of the build_risks_geojson_and_colorbar. I could not find any reason for which we would want to read the file each time we re-instantiate the colorbar and the GeoJSON objects. Especially since scores are still determined randomly for now - no refreshing issue.

  • Variabilized the opacity_level in the build_risks_geojson_and_colorbar.

  • Wrote a function to instantiate the slider. It is an instantiation of the dcc.Slider, put inside a HTML div so that we can modify its width and eventually centred on the page.

  • Added Markdown separators before and after the slider, in the part of the script where we define the layout of the page.

utils.py

Slightly reduced the map height to leave space for the slider. It is also useful for the "satellite" button in the alerts dashboard.

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.

Thanks a lot for this nice PR @pechouc !

This slider seems to perfectly match the opacity filtering onto the map risk layer.

I'll review in depth soon 👍

@Akilditu
Copy link
Collaborator

Akilditu commented Oct 20, 2020

Questions

The slider currently looks like:

Capture d’écran 2020-10-20 à 21 37 46

  • What do you think of the format of the slider? NB - as it might not be clear on the picture: marks are just indicative and can be clicked directly by the user, but any value between 0 and 1 can be chosen with a step of 0.01.

Just checked locally from your branch everything's fine and smooth.

  • Would you like me to add it to the homepage or should we keep it only in the "Niveaux de Risque" dashboard for now?

I think that we can keep on adding changes on splitted on both /alerts and /risks and then merge within homepage. This step of merging would require a dedicated PR as it implies also changes / addition of callbacks for filtering purposes.

  • As you will see, for the callback, I could not find a "lighter" solution than re-instantiating the colorbar and the geojson object. I have tried to find another way as I suspect this is a disproportionately heavy operation regarding the importance of the feature itself but nothing else worked. At least, it seems to work fine locally and doesn't break other callbacks but if you have any other idea, I would be glad to try them out!

I agree it seems a bit straight forward but if no other option then we'll stick to it. Not possible to force style param scale_style change without re-instantiating both objects right ? I will try to dig into it 😅

@pechouc
Copy link
Collaborator Author

pechouc commented Oct 21, 2020

I think that we can keep on adding changes on splitted on both /alerts and /risks and then merge within homepage. This step of merging would require a dedicated PR as it implies also changes / addition of callbacks for filtering purposes.

Very much agreed!

I agree it seems a bit straight forward but if no other option then we'll stick to it. Not possible to force style param scale_style change without re-instantiating both objects right ?

At first, I tried to only change the scale_style parameter but I could not find a way to call it in the callback function: it does not display any id which we could rely on and adding an arbitrary id parameter did not seem to make it work (seems like this works only for Dash components).

Then, I tried to only modify the hideout attribute of geojson_risks. The goal was to instantiate a new one and inject it into the GeoJSON object. I think that the callback was responding well but I encountered an issue with - I suppose - the Leaflet library which seemed to lack a parameter.

And finally, going one step above to the map object seemed to work. But let me know if you have any other idea!

Copy link
Contributor

@nrslt nrslt left a comment

Choose a reason for hiding this comment

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

Nice job @pechouc !

I completely agree with @Akilditu when considering keeping on working on the risk and alerts maps separately before considering merging everything.

Nice to see the progression !

@frgfm frgfm added this to the 0.1.0 milestone Oct 21, 2020
@frgfm frgfm added the type: enhancement New feature or request label Oct 21, 2020
@pechouc
Copy link
Collaborator Author

pechouc commented Oct 21, 2020

@Akilditu, do you prefer that I close this PR and merge the branch into master (possibly as a first version) or that I leave it open for you to have time to dig deeper?

@Akilditu
Copy link
Collaborator

@Akilditu, do you prefer that I close this PR and merge the branch into master (possibly as a first version) or that I leave it open for you to have time to dig deeper?

Did not find anything better, ok to close & merge !

@pechouc pechouc merged commit eef61c0 into master Oct 21, 2020
@pechouc pechouc deleted the opacity_slider branch October 21, 2020 18:15
nrslt referenced this pull request in nrslt/pyronear-webapp Oct 30, 2020
Added the slider and the callback for interactivity (#8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants