Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add ScreenGrid tooltip, implement in building damage map #1145

Merged
merged 5 commits into from
Nov 22, 2019

Conversation

jaronheard
Copy link
Contributor

Implements a ScreenGrid tooltip for cards/earthquake-damage-estimates-for-buildings-in-tillamook-county.

trimmed

data={data.damageEstimates.value}
data={data.damageEstimates.value.filter(
feature => feature.properties[mapStyles[dataType].field]
)}
Copy link

Choose a reason for hiding this comment

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

In the future, it's nice when values like this are abstracted to a well named variable so that it's clear how the data has been transformed.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM 👍

getProperty(
tooltipInfo[el.field] / tooltipInfo[el.totalField],
el.formatField
)}
Copy link
Member

Choose a reason for hiding this comment

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

In the section above, it looks like you're calculating the average using cellWeight divided by cellCount. But couldn't you just set the new aggregation prop to "MEAN" and achieve the same results? Why hard code this here?

@mendozaline
Copy link
Member

The new tooltip feature for the Screen Grid Map looks great!

I did notice some issues with how the tooltip is implemented in the Tillamook County Buildings card:

  1. There are colorless grid cells that somehow still produce a tooltip.
  • This doesn't seem like the correct behavior. However, it looks like this was an issues before this feature was added, since the same problem exists in the current live version.
  • I think this is an issue with the colorDomain the Screen Grid Map is using. Since the Screen Grid Map component isn't providing its own prop value, it's using the default min value of 1. Because ComLossRatio and ResLossRatio are decimals when these values are summed if the result isn't greater than 1, it's not being given a color.
  • The fastest way to fix this would be to multiply the values by 100 either on the backend or in the front end.
  1. There are grid cells that are a lighter color but have greater values than other darker cells that have lower values.
  • I think this is due to the mismatch between how the aggregated summed values of ComLossRatio or ResLossRatio are being used for the colors, but the tooltip is displaying the average loss ratio.
  • It's unclear what's being visualized. Displaying 2 different things may be the intended result, but shouldn't both the grid cells and the tooltip info match?
  • This can be fixed by setting the new aggregation prop to "MEAN" instead of "SUM", so both the colors and the tooltip match.
  1. The colorRange prop should only take an array of 6 colors, according to the deck.gl documentation.
  • I think this is why the map doesn't display all 9 colors from the CIVIC thermal color scheme. It may be only using the first six colors in the array. This could be why the map appears so light

When these issues are fixed, the map now looks like this:
Screen Shot 2019-11-11 at 20 36 50

There are no more colorless grid cells, the lower avg. values appear lighter and the bigger avg. values appear darker, and the full range of colors are now displayed.

@jaronheard
Copy link
Contributor Author

@mendozaline thanks for the thorough review!

I think I've resolved your comments -- however, I created issue #1152 to address the colorRange array length issue, and have a temporary implementation for now.

I reworded the language and I think that what is being visualized may be clearer now, but I'd love suggestions on how to make it even better.

Copy link
Member

@mendozaline mendozaline left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for making those changes.

@jaronheard jaronheard merged commit 1a89611 into master Nov 22, 2019
@jaronheard jaronheard deleted the add-screen-grid-map-tooltip branch November 22, 2019 03:59
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants