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

Feature/future carbon gains #3681

Merged
merged 12 commits into from
Jan 15, 2019
Merged

Feature/future carbon gains #3681

merged 12 commits into from
Jan 15, 2019

Conversation

dfrico
Copy link
Contributor

@dfrico dfrico commented Jan 4, 2019

Overview - Future Carbon Gains widget

This one is a version of the Potential tree biomass gain widget for the climate tab. It features a stacked bar chart to show Potential tree biomass gain of Young Secondary Forests, Mid-Age Secondary Forests, Pasture Area, and Crops.

@dfrico dfrico added the WIP Any PR that should not be merged to develop label Jan 4, 2019
@dfrico dfrico requested a review from edbrett January 4, 2019 16:11
@dfrico
Copy link
Contributor Author

dfrico commented Jan 4, 2019

I'm getting a "Network Error" on every request, maybe there is some CORS issues between the domains climate.globalforestwatch.org and www.globalforestwatch.org ?

@simaob
Copy link
Contributor

simaob commented Jan 4, 2019

can you share an example request?

@dfrico
Copy link
Contributor Author

dfrico commented Jan 4, 2019

Sure! This is one example. You can access it from the browser, and even the network tools are giving me 200s and 304s status codes, but I can't seem to get the data in the widget itself.

@edbrett edbrett added the staging Any PR expected to be kept on staging label Jan 9, 2019
@dfrico dfrico added develop Any PR that is ready to be merged to develop and removed WIP Any PR that should not be merged to develop labels Jan 9, 2019
@benlaken benlaken self-requested a review January 10, 2019 09:53
Copy link
Contributor

@benlaken benlaken left a comment

Choose a reason for hiding this comment

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

This is looking good, only noticed two small things:

  • There are currently no units on the pop-up (See image):

screenshot 2019-01-10 at 10 52 59

  • You have written 'carbon dioxide ' in the dynamic sentence, but you should be consistent between widgets and keep it as CO₂ instead.

@edbrett edbrett added WIP Any PR that should not be merged to develop and removed develop Any PR that is ready to be merged to develop labels Jan 10, 2019
Copy link
Contributor

@benlaken benlaken left a comment

Choose a reason for hiding this comment

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

LGTM!

@dfrico dfrico added develop Any PR that is ready to be merged to develop and removed WIP Any PR that should not be merged to develop labels Jan 15, 2019
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.

Great job! Nice tidy work. No significant comments but but review. 🌵 〽️

@@ -0,0 +1,50 @@
export default {
widget: 'futurecarbongains',
Copy link
Contributor

Choose a reason for hiding this comment

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

can we camelCase this like other widgets?

if (isEmpty(data)) return null;
const years = {};
const selectedData = data[settings.unit];
Object.keys(selectedData).forEach(key =>
Copy link
Contributor

Choose a reason for hiding this comment

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

you could use a reduce here to make this simpler.

label: labels[k] ? labels[k] : k,
color: colors.ramp && colors.ramp[i],
unit: 't',
unitFormat: num => formatNumber({ num, unit: '' })
Copy link
Contributor

Choose a reason for hiding this comment

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

This is ugly, but appreciated we dont have too many options right now. Going to make a note to improve this.

@edbrett edbrett merged commit 576e770 into develop Jan 15, 2019
@edbrett edbrett deleted the feature/future-carbon-gains branch January 15, 2019 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
develop Any PR that is ready to be merged to develop staging Any PR expected to be kept on staging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants