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

Fixed Tree Cover Pies #3571

Merged
merged 8 commits into from
Aug 31, 2018
Merged

Fixed Tree Cover Pies #3571

merged 8 commits into from
Aug 31, 2018

Conversation

01painadam
Copy link
Contributor

Overview

For any selected indicator (forest type / land category) the pies should show:

a) forest cover in that indicators polygon
b) forest NOT in that indicators polygon
c) Non-forest

e.g.

screen shot 2018-08-27 at 14 30 31

This fixes this and also introduces:

  • dynamic legend ("Other tree cover in ")

It also fixes other mathematical errors in calculating areas (see: mangroves)

Testing

Please test for indicator combination at iso/adm1/2 levels (Global not affected)

let plantations = 0;
let data = {};
if (extent.length) {
totalArea = adminExtent[0].total_area;
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to check admin response is defined here and below too otherwise it might crash the app.

const plantationsData =
plantationsResponse.data && plantationsResponse.data.data;
plantations = plantationsData.length
? plantationsData[0].value
Copy link
Contributor

Choose a reason for hiding this comment

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

same here.

plantationsResponse => {
const plantationsData =
plantationsResponse.data && plantationsResponse.data.data;
plantations = plantationsData.length
Copy link
Contributor

Choose a reason for hiding this comment

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

If you check for length on a undefined your gonna have a bad time.

}
];
if (!indicator && hasPlantations) {
if (indicator) {
parsedData.splice(1, 0, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to mutate the array?

color: colors.otherCover,
percentage: otherCover / totalArea * 100
});
} else if (!indicator && hasPlantations) {
parsedData.splice(1, 0, {
Copy link
Contributor

Choose a reason for hiding this comment

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

This might need updating also.

Copy link
Contributor

@edbrett edbrett left a comment

Choose a reason for hiding this comment

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

Good to have this fixed. You are creating a lot of situations where undefined could crash the app, particularly with arrays. Just make sure to check that [0] references are definitely array before doing so.

@edbrett edbrett added the staging Any PR expected to be kept on staging label Aug 29, 2018
@edbrett edbrett merged commit 5b5009d into develop Aug 31, 2018
@edbrett edbrett deleted the fix/land-cover-segments branch August 31, 2018 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
staging Any PR expected to be kept on staging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants