search

ua-snap / mapventure

Frontend for maps built with GeoServer
3 stars 0 forks source link

Update fire popup to use new fields, handle missing data better #337

Closed brucecrevensten closed 7 years ago

brucecrevensten commented 7 years ago

This adds some checks and doesn't display data that is missing (empty).

This change depends on https://github.com/ua-snap/mv-aicc-fire-shim/pull/11 in place before it will work! Ask me if you aren't sure how to test it.

This change is Reviewable

cstephen commented 7 years ago

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions.

app/scripts/maps/alaska-wildfires/controller.js, line 272 at r1 (raw file):


      var acres = fireInfo.acres + ' acres';
      var updated = _.isEmpty(fireInfo.updated) ? '' :

I'm planning to replace .isEmpty() with .isNull() here, too, to avoid confusion. (See comment for next line.)

app/scripts/maps/alaska-wildfires/controller.js, line 274 at r1 (raw file):

      var updated = _.isEmpty(fireInfo.updated) ? '' :
        '<p class="updated">Updated ' + fireInfo.updated + '</p>';
      var out = _.isEmpty(fireInfo.outdate) ? '' : '<p class="out">Out date: ' + moment.utc(moment.unix(fireInfo.outdate / 1000)).format('MMMM Do, h:mm a') + '</p>';

There's a bug here. .isEmpty() evaluates as true for anything that's not an object, array, or string (because strings are character arrays, I'm guessing). We can use .isNull() instead.

app/scripts/maps/alaska-wildfires/controller.js, line 275 at r1 (raw file):

        '<p class="updated">Updated ' + fireInfo.updated + '</p>';
      var out = _.isEmpty(fireInfo.outdate) ? '' : '<p class="out">Out date: ' + moment.utc(moment.unix(fireInfo.outdate / 1000)).format('MMMM Do, h:mm a') + '</p>';
      var cause = _.isEmpty(fireInfo.cause) ? '' : '<h3>Cause: ' + fireInfo.cause + '</h3>';

I'm planning to replace .isEmpty() with .isNull() here, too, to avoid confusion.

Comments from Reviewable

cstephen commented 7 years ago

@brucecrevensten I'm comfortable merging this if my change looks good to you.

brucecrevensten commented 7 years ago

Craig, go ahead and merge this. I was going to take a look but I'm unclear on the review/merge status for the corresponding ticket in the mv-aicc-fire-shim repo, so I think I'd prefer you to conclude these two items.