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/fires within settings btn #3997

Merged
merged 8 commits into from May 26, 2020

Conversation

dfrico
Copy link
Contributor

@dfrico dfrico commented May 25, 2020

Overview

Adding a button to the 'fires within' widget to open the settings like we did in the Cumulative Fire Alerts widget. This button should be shown in any pie chart widget when:

  • showSettingsBtn: true is in the settings object (in index.js)
  • no forest type or land category is selected (and its default values are empty)

image

@dfrico dfrico requested a review from edbrett May 25, 2020 14:47
@dfrico dfrico self-assigned this May 25, 2020
@vercel
Copy link

vercel bot commented May 25, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/vizzuality1/gfw-docs/6sjozt8pf
✅ Preview: https://gfw-docs-git-feature-fires-within-settings-btn.vizzuality1.now.sh

@edbrett edbrett temporarily deployed to gfw-feature-fires-withi-0hcek4 May 25, 2020 14:48 Inactive
@dfrico dfrico changed the base branch from develop to feature/fires May 25, 2020 14:48
@dfrico dfrico temporarily deployed to gfw-feature-fires-withi-0hcek4 May 25, 2020 14:57 Inactive
@01painadam
Copy link
Contributor

Looks good to me dude 👌

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.

Lets make that button more configurable

className="pie-contextual-settings-btn"
onClick={() => toggleSettingsMenu()}
>
+ Select an intersection
Copy link
Contributor

Choose a reason for hiding this comment

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

this isnt generic, a widget might not have intersections but want this button to exist. It should be configurable.

@@ -23,7 +23,10 @@ class ChartTooltip extends PureComponent {
: values[d.key];

return hideZeros && (!values || !value) ? null : (
<div key={d.key} className={`data-line ${d.position || ''}`}>
<div
key={d.key ? d.key : `setting-${i}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

no use of i in keys please. React gets very upset about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This solves an issue where the key is undefined and react complains about it. Not sure what to add then 🙃

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't have a key we shouldn't be rendering it.

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no unique data to identify what it is then what is it doing there? Check the data and the render to tidy it up.

@dfrico dfrico requested a deployment to gfw-feature-fires-withi-0hcek4 May 26, 2020 11:56 Abandoned
@dfrico dfrico requested a deployment to gfw-feature-fires-withi-0hcek4 May 26, 2020 12:05 Abandoned
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.

Nice abstraction of the button to a function. Thanks for the tidy up.

@edbrett edbrett merged commit 371fc23 into feature/fires May 26, 2020
@edbrett edbrett deleted the feature/fires-within-settings-btn branch May 26, 2020 12:11
@edbrett edbrett temporarily deployed to gfw-feature-fires-withi-0hcek4 May 26, 2020 12:11 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants