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

Add CSV Download button to reports (LG-5913) #57

Merged
merged 10 commits into from
Jun 17, 2022
Merged

Conversation

zachmargolis
Copy link
Contributor

@zachmargolis zachmargolis commented Jun 17, 2022

  • Reuses table data as CSV, with data-csv attribute for
    providing raw data

Screen Shot 2022-06-17 at 9 25 42 AM

- Reuses table data as CSV, with `data-csv` attribute for
  providing raw data
@zachmargolis zachmargolis requested review from aduth and a team June 17, 2022 18:45
<a
className="usa-button usa-button--outline"
download="report.csv"
href={`data:text/csv;charset=utf-8,${encodeURIComponent(toCSV(data))}`}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for most of our pages, the tables are not too big, but this is adding a big-ish string and just letting it hang out in the DOM. maybe if this becomes slow we could make this an async operation

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Very cool!


export type TableRow = (string | number | VNode)[];
export type TableCell = string | number | VNode;
Copy link
Member

Choose a reason for hiding this comment

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

You might be able to avoid some of the forced type-casting below by specifying the props shape of the VNode.

Suggested change
export type TableCell = string | number | VNode;
export type TableCell = string | number | VNode<CSVProps>;

(it's gonna require more changes than just the suggestion)

Copy link
Contributor Author

@zachmargolis zachmargolis Jun 17, 2022

Choose a reason for hiding this comment

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

I gave it a shot and colSpan still needed a cast somehow, and these changes caused typechecker errors in the tests where we do some casting too, going to skip for now, but here was my attempted patch

diff --git a/src/components/table.tsx b/src/components/table.tsx
index 39097b7..2633b65 100644
--- a/src/components/table.tsx
+++ b/src/components/table.tsx
@@ -2,7 +2,12 @@ import { VNode, render } from "preact";
 import { csvFormatValue } from "d3-dsv";
 import Icon from "./icon";
 
-export type TableCell = string | number | VNode;
+interface CSVProps {
+  "data-csv": string[];
+  colSpan: number;
+}
+
+export type TableCell = string | number | VNode<CSVProps>;
 export type TableRow = TableCell[];
 
 export interface TableData {
@@ -49,24 +54,19 @@ function Row({
  */
 const doc = document.implementation.createHTMLDocument("");
 
-function textContent(v: VNode): string {
+function textContent(v: VNode | VNode<CSVProps>): string {
   render(v, doc.body);
   return doc.body.textContent || "";
 }
 
-interface CSVProps {
-  "data-csv": string[];
-  colSpan: number;
-}
-
 function toCSVValues(cell: TableCell): string[] {
   if (typeof cell === "object") {
     if ("data-csv" in cell.props) {
-      return (cell.props as unknown as CSVProps)["data-csv"];
+      return cell.props["data-csv"];
     }
 
     const text = textContent(cell);
-    const colspan = (cell.props as unknown as CSVProps).colSpan || 1;
+    const colspan = (cell.props as CSVProps).colSpan || 1;
 
     const empties = Array(colspan - 1).fill("");

Copy link
Member

Choose a reason for hiding this comment

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

Any luck with at least VNode<any> ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

our typescript config said no:

  5:49  error  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

Copy link
Member

Choose a reason for hiding this comment

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

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'll clean that up in a second PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

follow-up: #58

src/components/table.tsx Outdated Show resolved Hide resolved
src/components/table.tsx Outdated Show resolved Hide resolved
src/components/icon.tsx Outdated Show resolved Hide resolved
@@ -39,3 +39,18 @@
overflow: hidden;
white-space: nowrap;
}


// Borrowed from: https://github.com/18F/identity-idp/blob/main/app/assets/stylesheets/components/_icon.scss

Choose a reason for hiding this comment

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

code looks clear. not a fan of all the comments

Copy link

@stevegsa stevegsa 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!

zachmargolis and others added 6 commits June 17, 2022 12:07
lowercase url

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
Array#fill

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
Co-authored-by: Andrew Duthie <aduth@users.noreply.github.com>
zachmargolis and others added 3 commits June 17, 2022 12:23
replaceNode = true

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
- typescript was mad at the "true" value
- tests caught a regression in cell content
@zachmargolis zachmargolis merged commit b4192dc into main Jun 17, 2022
@zachmargolis zachmargolis deleted the margolis-csv-export branch June 17, 2022 19:32
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.

3 participants