LiveBy / react-leaflet-choropleth

Component for React-Leaflet that adds choropleth functionality
ISC License
16 stars 8 forks source link

How can I get the labels to update on state change? #2

Closed davidascher closed 8 years ago

davidascher commented 8 years ago

TL;DR: tooltips aren't regenerated when state changes.

I have a working (yea!) use of react-leaflet and react-leaflet-choropleth, which uses geoJSON data and creates a cloropleth map based on an elasticsearch query. The elasticsearch query is triggered on the componentWillMount() method of the react component which includes the choropleth.

The render method is simple:

    <Map styleName="map" center={position} zoom={1}>
      <TileLayer
        url={mapURL}
        attribution={attribution}
        maxZoom={maxZoom}
      />
      <Choropleth
        data={geojson}
        valueProperty={this.numCasesPerCountry}
        scale={scale}
        visible={this.isCountryListed}
        onEachFeature={(feature, layer) => layer.bindPopup(this.labelPerCountry(feature))}
        steps={5}
        mode='e'
        style={style}
      />
    </Map>

And I have callbacks defined for all three methods. The problem is that when the component state changes, the feature color is updated, but the label is not, as indicated by this screengrab, which shows that Italy has a high number of cases, is colored appropriately, but its tooltip isn't up to date:

screenshot 2016-05-30 15 01 53

Note that the labelPerCountry is only called once.

Am I right that this is a bug? I suspect that react-leaflet's third technical consideration:

The components exposed are abstractions for Leaflet layers, not DOM elements. Some of them have properties that can be updated directly by calling the setters exposed by Leaflet while others should be completely replaced, by setting an unique value on their key property so that they are properly handled by React's algorithm.

is relevant, but I haven't dug into either codebase to unpack it.

davidascher commented 8 years ago

I'm guessing this bug isn't w/ choropleth but with my usage of react-leaflet, now that I look at the code a bit deeper.

jgimbel commented 8 years ago

onEachFeature={(feature, layer) => layer.bindPopup(this.labelPerCountry(feature))} is most likely your problem.

onEachFeature will only be run when the leafletElement is mounted (Reference). Hence the popup not updating on each render call. Instead the common solution is to use the Popup component as a child of geojson, so on each render the popup component will checks whether it should be updated(Example). The problem here is that you are not able to do something as simple as adding the children...

<Choropleth
  data={geojson}
  valueProperty={this.numCasesPerCountry}
  scale={scale}
  visible={this.isCountryListed}
  steps={5}
  mode='e'
  style={style}
>
  <Popup>
    {geojson.properties.name /*bad because the geojson is an array */}
  </Popup>
</Choropleth>

or mapping through all the geojson features.

//bad because this creates multiple choropleths, and data is not shared between.
geojson.map(country => ( 
<Choropleth
  data={geojson}
  valueProperty={this.numCasesPerCountry}
  scale={scale}
  visible={this.isCountryListed}
  steps={5}
  mode='e'
  style={style}
>
  <Popup>
    {contry.properties.name}
  </Popup>
</Choropleth>
)

The right solution will require the way we pass props to change, so that children recieve the feature they are rendered inside. (Like react-leaflet#mapLayer)

const CountryName = ({ feature }) => (
  <Popup>
    {feature.properties.name}
  <Popup>
)

...

<Choropleth
  data={geojson}
  valueProperty={this.numCasesPerCountry}
  scale={scale}
  visible={this.isCountryListed}
  steps={5}
  mode='e'
  style={style}
>
  <CountryName />
</Choropleth>
davidascher commented 8 years ago

Apologies for the noise.

Fixed it, by using code like:

  onEachFeature(feature, layer) {
    let labelPerCountry = this.labelPerCountry;
    layer.on({
      click: function(event) {
        var popup = L.popup()
            .setLatLng(event.latlng)
            .setContent(labelPerCountry(feature))
            .openOn(layer._map);

          }
    });
  }

and:

      <Choropleth
        data={geojson}
        valueProperty={this.numCasesPerCountry}
        scale={scale}
        visible={this.isCountryListed}
        onEachFeature={this.onEachFeature}
        steps={5}
        mode='e'
        style={style}
      />

Is there anything that you see as problematic w/ the above?

jgimbel commented 8 years ago

Seems like an elegant way of handling the tooltip without dealing with flux store/state. I am going to work on allowing children as a option for anyone that might need it later.

jgimbel commented 8 years ago

Children passed to the choropleth component are now passed to each GeoJSON component. To get the current shape, feature is assigned in its props. This makes the solution I mentioned earlier now available, in case you need to use something like state or flux/redux to handle changes.