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

Fix for TimeSliderChoropleth control breaking when layer turned off and on #1380

Conversation

markhudson42
Copy link
Contributor

This is a fix for issue #1379 which shows that turning a TimeSliderChoropleth layer off and on again using the standard LayerControl causes the Choropleth to be broken and for the coloration not to be applied anymore.

I believe that the reason for this is that the colouring is controlled by ID strings, such as 'feature-0', which are defined for each GeoJSON feature but in the code rather than in the feature data itself. When the layer is turned off and on again, all these ID strings are lost, which is why the coloration no longer works.

In my solution I capture the Leaflet overlayadd event, and re-run some of the code to add the IDs again, draw the original dotted outlines for the features, and to call the fill_map() function again to do the initial coloration.

I had provided more information in my original comment to the original github issue #1379 .

Mark.

Copy link
Member

@Conengmo Conengmo left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this! I haven't tried this myself yet, but your explanation makes sense.

You copied some code into the new onOverlayAdd function. That's fine, but could you then replace the code you copied by a call to this function? Or perhaps that old code isn't even necessary because the overlayadd event is called on map creation?

After we resolved that, the next steps are me testing this and then merging it if it indeed works as expected.

@markhudson42
Copy link
Contributor Author

markhudson42 commented Aug 26, 2020

Thanks for looking into this! I haven't tried this myself yet, but your explanation makes sense.

You copied some code into the new onOverlayAdd function. That's fine, but could you then replace the code you copied by a call to this function? Or perhaps that old code isn't even necessary because the overlayadd event is called on map creation?

After we resolved that, the next steps are me testing this and then merging it if it indeed works as expected.

Hi, thanks for the sensible suggestions. I wasn't thinking very efficiently that day.

I've found that the overlayadd event is not called on map creation, so I have added a call to the onOverlayAdd function in place of the code that I had copied.

So now we have:

            function onOverlayAdd(e) {
                {{ this.get_name() }}.eachLayer(function (layer) {
                    layer._path.id = 'feature-' + layer.feature.id;
                });

                d3.selectAll('path')
                .attr('stroke', 'white')
                .attr('stroke-width', 0.8)
                .attr('stroke-dasharray', '5,5')
                .attr('fill-opacity', 0);

                fill_map();
            }
            {{ this._parent.get_name() }}.on('overlayadd', onOverlayAdd);
            
            onOverlayAdd(); // fill map as layer is loaded

where I've modified the order of some of the lines.

I'll do the push and pull again.

Mark.

@paulsp94
Copy link

paulsp94 commented Sep 9, 2020

Hey awesome work, really appreciate all the effort you put into fixing the problem.
Any news when this fix could be published in folium?

@Conengmo Conengmo merged commit 16a300e into python-visualization:master Nov 7, 2020
@Conengmo
Copy link
Member

Conengmo commented Nov 7, 2020

Thanks so much for your work on this @markhudson42 ! And great issue and PR description by the way.

@jsmrcina
Copy link

jsmrcina commented Jan 2, 2022

@Conengmo @markhudson42 I can still repro this if the TimeSliderChoropleth is under a FeatureGroup. As soon as I move it to be a top-level child of the map, it works fine. But under the FeatureGroup, the behavior is exactly as @markhudson42 describes above (disabling the layer causes the choropleth to be broken as described). Is that expected?

@Conengmo
Copy link
Member

Conengmo commented Jan 4, 2022

Plugins can have unexpected behavior when interacting with other components. Especially in the case of TimeSliderChoropleth it doesn't surprise me that much.

@markhudson42 markhudson42 deleted the time_slider_choropleth_layer_control_fix branch January 20, 2024 14:49
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.

5 participants