Skip to content
This repository has been archived by the owner. It is now read-only.

Clean up heatmap component #25

Merged
merged 1 commit into from Aug 23, 2017

Conversation

Projects
None yet
3 participants
@c-w
Copy link
Member

commented Aug 22, 2017

Just some assorted cleanups:

  • Make whitespace consistent (always use spaces, never tabs)
  • Make indentation consistent (always use two spaces)
  • Make variable declaration consistent (one let/const per variable)

Additionally, call out via the // FIXME comments:

  • one potential bug
  • one configuration opportunity
  • one potential security issue

@c-w c-w requested review from Smarker and erikschlegel Aug 22, 2017

@c-w

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2017

@erikschlegel Would be great if you could feed back on the // FIXME comment that I added on lines 28, 29 and 147 which points to potential issues with the v1 code. Everything else can be reviewed by someone else.

@c-w

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2017

NB: here's a minimal example of how we'd integrate @timfpark's rhom-webapp heatmap into Fortis, but we can't use that since Fortis uses bare leaflet and not leaflet-react.

This is just here for documentation/posteriority, so no need to review the code in the remainder of this comment unless you're really interested in the heatmap feature.

import async from 'async';
import React from 'react';
import Tile from 'geotile';
const HeatmapOverlay = require('leaflet-heatmap');

class ReactHeatmapLayer extends React.Component {
  constructor(props) {
    super(props);
  }

  componentDidMount() {
    this.heatmapLayer = new HeatmapOverlay(this.props.config);
    this.props.map.addLayer(this.heatmapLayer);
  }

  componentDidUpdate(prevProps, prevState) {
    this.heatmapLayer.setData(this.props.heatmap);
  }

  render() {
    return null;
  }
}

const config = {
  latField: 'latitude',
  lngField: 'longitude',
  maxOpacity: 0.3,
  radius: 15,
  scaleRadius: false,
  valueField: 'count'
};

const BASE_HEATMAP_VALUE = 50;
const MAX_ZOOM = 18;

class LocationHeatmap extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      heatmap: {
        max: 1,
        data: []
      }
    };
  }

  componentDidMount() {
    this.rebuildHeatmap();
  }

  buildHeatmap(heatmaps) {
    let min;
    let max;
    let average = 0.0;
    let count = 0;
    let counts = [];
    let mean = 0.0;
    let maxClamp = 1.0;

    let points = [];

    heatmaps.forEach(heatmap => {
      Object.keys(heatmap).forEach(tileId => {
        count += 1;
        let tile = Tile.tileFromTileId(tileId);
        let tileCount = heatmap[tileId];
        points.push({
          latitude: tile.centerLatitude,
          longitude: tile.centerLongitude,
          count: tileCount + BASE_HEATMAP_VALUE
        });
        counts.push(tileCount);
        average += tileCount;
        if (!min || tileCount < min)
          min = tileCount;
        if (!max || tileCount > max)
          max = tileCount;
      });
    });

    counts.sort( (a,b) => {
      return a - b;
    });

    if (count > 0) {
      average /= count;
      mean = counts[Math.floor(counts.length / 2)];
      maxClamp = counts[Math.floor(0.7 * counts.length)];
    }

    this.setState({
      heatmap: {
        max: maxClamp,
        data: points
      }
    });
  }

  fetchHeatmapClusters(callback) {
    // returns [{string -> number}]
    //           tileId    count
    callback(null, null);
  }

  rebuildHeatmap() {
    if (!this.props.bounds) {
      return;
    }

    this.fetchHeatmapClusters((err, heatmapClusters) => {
      if (err) return;

      this.buildHeatmap(heatmapClusters);
    });
  }

  shouldRebuildHeatmap(prevProps, prevState) {
    return !(
      prevProps && prevProps.bounds &&
      this.props.bounds.north === prevProps.bounds.north &&
      this.props.bounds.south === prevProps.bounds.south &&
      this.props.bounds.east === prevProps.bounds.east &&
      this.props.bounds.west === prevProps.bounds.west
    );
  }

  componentDidUpdate(prevProps, prevState) {
    if (!this.shouldRebuildHeatmap(prevProps, prevState)) {
      return console.log('no relevant change, not rebuilding heatmap');
    }

    this.rebuildHeatmap();
  }

  render() {
    if (!this.props.map) return null;

    return (
      <ReactHeatmapLayer
        config={config}
        heatmap={this.state.heatmap}
        map={this.props.map}
      />
    );
  }
}

class Page extends React.Component {
  render() {
    return (
      <Map
        center={[parseFloat(this.state.latitude), parseFloat(this.state.longitude)]}
        onMoveend={this.handleMoveEnd}
        ref='map'
        style={mapStyle}
        zoom={parseFloat(this.state.zoom)}
        zoomControl={false}>
        
        <LocationHeatmap
          bounds={this.state.bounds}
          map={this.refs.map}
          zoom={this.state.zoom}
        />
        
      </Map>
    );
}
Clean up heatmap component
Just some assorted cleanups:
- Make whitespace consistent (always use spaces, never tabs)
- Make indentation consistent (always use two spaces)
- Make variable declaration consistent (one let/const per variable)

Additionally, call out via the `// FIXME` comments:
- one potential bug
- one confiuration opportunity
- one potential security issue

@c-w c-w force-pushed the cleanup-heatmap branch from 28cf16e to 2b7f159 Aug 22, 2017

@Smarker
Copy link
Contributor

left a comment

LGTM - with comments

let sentimentClass = "";
let sentimentIcon = "";

if (sentimentLevel < 30) {

This comment has been minimized.

Copy link
@Smarker

Smarker Aug 22, 2017

Contributor

Could make 30, 55, and 80 into a constant.

This comment has been minimized.

Copy link
@c-w

c-w Aug 22, 2017

Author Member

Yeah, definitely. I'll make a second pass which will focus on extracting constants/methods. Wanted to keep the first round minimal to make reviewing easier (whitespace changes mess up diffs!).

let infoBoxInnerHtml =
`<div id="sentimentGraph">
<div class="sentimentGraphBar ${sentimentClass}">
${parseFloat((sentimentLevel / 100) * 10).toFixed(2)} / 10

This comment has been minimized.

Copy link
@Smarker

Smarker Aug 22, 2017

Contributor

Would it be possible to put this into a named function so you don't have to try to understand what ${parseFloat((sentimentLevel / 100) * 10).toFixed(2)} / 10 does?

This comment has been minimized.

Copy link
@c-w

c-w Aug 22, 2017

Author Member

Definitely. As mentioned above: will make a second pass which will focus on extracting constants/methods. Wanted to keep the first round minimal to make reviewing easier (whitespace changes mess up diffs!).

This comment has been minimized.

Copy link
@Smarker

Smarker Aug 22, 2017

Contributor

ok good point!

@erikschlegel
Copy link
Collaborator

left a comment

LGTM

const PARELLEL_TILE_LAYER_RENDER_LIMIT = 200;
const SENTIMENT_FIELD = 'neg_sentiment';
const defaultClusterSize = 40;
const DEFAULT_LANGUAGE = 'en';

const MAPBOX_ACCESS_TOKEN = 'pk.eyJ1IjoiZXJpa3NjaGxlZ2VsIiwiYSI6ImNpaHAyeTZpNjAxYzd0c200dWp4NHA2d3AifQ.5bnQcI_rqBNH0rBO0pT2yg'; // FIXME: should this really be checked in?

This comment has been minimized.

Copy link
@erikschlegel

erikschlegel Aug 23, 2017

Collaborator

I agree that this is far from ideal, but we need to define the workflow around how this should be maintained from a user experience standpoint.


getSentimentColor(sentiment){
return Actions.constants.SENTIMENT_COLOR_MAPPING[sentiment];
if (filters > 0) { // FIXME: isn't filters a list? if so: this would always return false

This comment has been minimized.

Copy link
@erikschlegel

erikschlegel Aug 23, 2017

Collaborator

not sure. we should test this in the debugger.

@c-w c-w merged commit 4a1373a into master Aug 23, 2017

@c-w c-w deleted the cleanup-heatmap branch Aug 23, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.