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/remove widget dependencies #3512

Merged
merged 11 commits into from
Jul 30, 2018

Conversation

pjosh
Copy link
Contributor

@pjosh pjosh commented Jul 16, 2018

No description provided.

@pjosh pjosh requested a review from edbrett July 16, 2018 13:18
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.

Looks good buddy! I have some widgets (loss and FAO) not working at global level on the dashboards. Could you take a look?

@@ -53,7 +52,9 @@ const mapStateToProps = ({ widgets, location }, ownProps) => {
const locationPath = `dashboards/${type || 'global'}/${locationUrl}`;
const widgetQuery = `widget=${widget}`;
const widgetState =
query && query[widget] ? `&${widget}=${query[widget]}` : '';
query && query[widget]
? `&${widget}=${btoa(JSON.stringify(query[widget]))}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ha! You should no longer need to do this. You can now find this state as a clean and tidy object on the location.query.widgets reducer in the store. You can use a selector to merge the props just like in the map!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't do it to update the current url. Actually it's only for generate the share url that we need to pass to the EXPLORE ON GFW button on the embed view.

@pjosh
Copy link
Contributor Author

pjosh commented Jul 23, 2018

@edbrett the FAO widget has failed because this branch was outdated with develop and had the 1 = 1 sql issue. Now it's fixed!
About the Loss widget, yeah it fails, but the issue was in other PR that we already merge 🤦‍♂️ . I've fixed it directly in feature/map-v2 6092880 and now is up to date here.

@edbrett edbrett merged commit ff5a609 into feature/map-v2 Jul 30, 2018
@edbrett edbrett deleted the feature/remove-widget-dependencies branch August 2, 2018 08:26
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.

2 participants