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

Exposure Analysis Download CSV access from anywhere #920

Merged

Conversation

HarryMytilinaios
Copy link
Contributor

This PR adds the Download CSV functionality for the Exposure Analysis both in the Layer Accordion component and in the Legend component, giving us access to download it from both places as well as the the ExposureAnalysisActions below the table. The sorting functionality is also ported to all places.

@github-actions
Copy link

github-actions bot commented Jul 14, 2023

Build succeeded and deployed at https://prism-920.surge.sh
(hash d1c8623 deployed at 2023-07-20T12:36:36)

@HarryMytilinaios
Copy link
Contributor Author

We can actually access now the Download CSV for the enabled exposure analysis from the layers panel or from Legends component see the screenshots below. The Sorting functionality is also enabled to all places.

Screenshot from 2023-07-14 19-51-58
Screenshot from 2023-07-14 19-52-07

@wadhwamatic
Copy link
Member

Nice work on this @HarryMytilinaios! This is very much needed. We need to harmonize this with the UI that @hafidz142 has worked in PR #919. I mentioned the same to @ericboucher. Thanks for syncing up on this with them next week

@wadhwamatic
Copy link
Member

wadhwamatic commented Jul 19, 2023

@HarryMytilinaios - I sync'd with @ericboucher about this earlier. Let's go ahead and merge this and we'll have @hafidz142 update his PR afterwards

Pending @ericboucher's approval!

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ This pull request was sent to the PullRequest network.


@HarryMytilinaios you can click here to see the review status or cancel the code review job - or - cancel by adding [!pr] to the title of the pull request.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest Breakdown

Reviewable lines of change

+ 261
- 101

72% TSX
26% TypeScript
2% Jest Snapshot (tests)

Type of change

Feature - These changes are adding a new feature or improvement to existing code.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

Overall, this set of changes is looking good, nice work! I've left one small recommendation that could optionally be applied before merging. No major issues stood out.

Image of Mark M Mark M


Reviewed with ❤️ by PullRequest

const analysisDefinition = useSelector(getCurrentDefinition);

const exposureAnalysisTableData = getExposureAnalysisTableData(
analysisData?.tableData as TableRow[],
Copy link

Choose a reason for hiding this comment

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

This optional chain implies that this expression might return undefined, but I see it's being cast as an always-truthy TableRow[]. Could a fallback empty array make this line less error-prone?

(analysisData?.tableData || []) as TableRow[],

A similar suggestion applies to the related changes in AnalysisDownloadButton.tsx in this PR.

🔹 Reduce Future Bugs (Nice to have)

Image of Mark M Mark M

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

No merge blocking issues found. Changes are in line with what is stated in the description of the PR. Thanks for the pull request.

Image of Frank T Frank T


Reviewed with ❤️ by PullRequest

@ericboucher ericboucher changed the title Exposure Analysis Download CSV access from anywhere; COUNTRY=cambodia Exposure Analysis Download CSV access from anywhere Jul 20, 2023
@ericboucher ericboucher merged commit 27d6a64 into master Jul 20, 2023
6 checks passed
@ericboucher ericboucher deleted the feature/download-csv-exposure-analysis-from-multiple-places branch July 20, 2023 15:14
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.

Revisit UX to download CSV results in an exposure analysis
3 participants