Add summary statistics to location details and rework column metadata types #1634

johnclary · 2024-12-18T19:12:22Z

Associated issues

Location details: add summary statistics to details card atd-data-tech#20354
Add support for rendering nested record values in tables and data cards atd-data-tech#20316

This PR restructures the typing and config structure for managing editable columns in the app. It makes our config + components more abstract, so that they can better handle arbitrary relationships rather than the hardcoded "lookup table" relationship.

One additional change that I would like to make, but have not in the interest of the code diff, is to stop using the word "column" where we really mean "field". This came up in some PR feedback from Chia and Rose recently.

Thank you for reviewing! Since this is so large, I'd be happy to set up a time to pair or review as a group interactively.

Testing

URL to test: Netlify

Steps to test:

Navigate to a location's details page and verify that the Details card is rendering normally.
Head over to a crash details page, and try editing a variety of fields, including fields that use a relationship (e.g. Collision Type), a yes/no field, and fields in the Units table.

Ship list

Check migrations for any conflicts with latest migrations in main branch
Confirm Hasura role permissions for necessary access
Code reviewed
Product manager approved

netlify · 2024-12-18T19:14:31Z

✅ Deploy Preview for vision-zero-nextjs ready!

Name	Link
🔨 Latest commit	`2195d11`
🔍 Latest deploy log	https://app.netlify.com/sites/vision-zero-nextjs/deploys/676987e8c0f100000833d778
😎 Deploy Preview	https://deploy-preview-1634--vision-zero-nextjs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

johnclary · 2024-12-20T16:15:18Z

app/app/locations/[location_id]/page.tsx

          <LocationMapCard location={location} />
        </Col>
-        <Col sm={12} md={4} className="mb-3">
+        <Col sm={12} md={6} lg={5} className="mb-3">


I like making the location map a bit wider than in the old VZE—just trying to strike a balance to prevent the details card form sidescrolling

johnclary · 2024-12-20T16:16:18Z

app/components/DataCard.tsx

+    editColumn?.editable && editColumn?.relationship
+      ? editColumn.relationship
+      : undefined
+  );


we only need to use the lookup query when a column is editable and it has relationship metadata

johnclary · 2024-12-20T16:20:00Z

app/components/DataCard.tsx

+    // Save the value to the foreign key column, if exists
+    const saveColumnName = editColumn.relationship?.foreignKey
+      ? editColumn.relationship?.foreignKey
+      : editColumn.path;


this hints at the awkward dance we need to do between rendering a value from an object relationship. E.g. crash. collsn.label versus editing that value, which requires us to update the foreign key at crash.fhe_collsn_id.

This PR is trying to iron this out by moving the necessary metadata into the column relationship config, whereas before we had some special lookup table logic hardcoded into various functions.

One important thing that these changes do is open up future iterations where we are editing relationships that are not just plain old lookup tables. E.g., if we wanted a user to select an EMS record from a list so that it can be linked to a crash.

ah, right—one other wrinkle (not new to this PR) is that in order to be able to edit lookup values our graphql queries need to include both the foreign key column and the related record itself.

For example, in the Crash query, you can see here that we have

# this is the foreign key to the lookups.collsn table fhe_collsn_id # this is the related collision type lookup value collsn { id label }

If you were to omit the foreign key from the query, the select input in the UI would never correctly show the currently selected lookup option. So it would be fairly obvious that something was wrong. This is not documented anywhere ☹️

thanks for pointing this out. i think this part of apps always gets a bit wrinkly, and I don't think that it is too bad to not have this documented and maybe even a bit intuitive? i also have not written any of this code so I could be wrong!

johnclary · 2024-12-20T16:23:07Z

app/components/RelatedRecordTableRow.tsx

i've mentioned this elsewhere, but it is rather unfortunate that the RelatedRecordTableRow is almost identical to the DataCard component except x + y axes are flipped. Would be cool to try to abstract this 🤷

johnclary · 2024-12-20T16:25:51Z

app/configs/chargeColumns.tsx

    label: "Citation Number",
  },
-};
+} satisfies Record<string, ColDataCardDef<Charge>>;


here are the docs about Typescript satisfies. it seems like it was designed for this exact scenario: it lets TypeScript infer this object's keys while still checking that each column config matches the ColDataCardDef

this change fixes a type bug down in the card/table configurations 👇

thanks for pointing to the docs for this one, and I agree that it fits this nicely. I played around with this in my editor, and I see the bug replicated when I go back to the previous typing on this config. sad about no pizza 🍕🥲 but happy about the TIL!

I liked this blog post about satisfies: https://fnune.com/typescript/2022/11/15/typescript-series-5-record-and-the-satisfies-operator/

johnclary · 2024-12-20T16:27:08Z

app/configs/crashDataCard.tsx

-    crashesColumns.fhe_collsn_id,
-    crashesColumns.rpt_city_id,
+    crashesColumns.collsn,
+    crashesColumns.city,


before adding that satisfies magic to the column indexes (see my other comment), there was no type checking on these column names. You could have added a column to the summary section called crashColumns.pizza and typescript would not have blinked.

johnclary · 2024-12-20T16:30:12Z

app/configs/crashesColumns.tsx

      tableSchema: "lookups",
      tableName: "collsn",
+      idColumnName: "id",
+      labelColumnName: "label",
+      foreignKey: "fhe_collsn_id",


here is the new Path type in action, along with relationship metadata. the very cool thing here is that the path property is type-checked against the Crash type. so "collsn.label" is valid but collsn.pizza would not be.

so this new Path type unlocks adding column metadata for nested properties in a typesafe way 🌈

this is so slick! love that the path notation feels like lodash .get() notation which i think is a good sign for this being intuitive. 🚀

johnclary · 2024-12-20T16:32:44Z

app/configs/locationColumns.ts

+  cr3_crash_count: {
+    path: "locations_list_view.cr3_crash_count",
+    label: "CR3 crashes (5 years)",
+  },


again, here you can see the path magic in action, so we can now render this nested value on the location details card. note how the relationship property is absent—we don't need it because this field is not editable.

johnclary · 2024-12-20T16:35:55Z

app/types/relationships.ts

+export type LookupTableOption = {
+  id: string | number;
+  label: string | number;
+};


so now we have the generic Relationship type which covers lookup tables and any other object relationship that a record might have.

and we still have the LookupTableOption to describe the id/label structure of our conventional lookup tables.

johnclary · 2024-12-20T16:39:18Z

app/types/utils.ts

yeah, this new Path type is definitely typescript hard mode. I link to a 3rd-party lib that does something similar, which I wouldn't mind reaching for but it does have more functionality than we need.

johnclary · 2024-12-20T16:43:45Z

app/utils/formHelpers.ts

@@ -75,47 +110,45 @@ const renderYesNoString = (value: unknown): string => {
 };

 /**
- * Return the `label` column from a related lookup table
- */
-export const renderLookupLabel = <T extends Record<string, unknown>>(


here's one of those functions that was hardcoded to deal with with our lookup tables in the lookups schema, but not for other types of relationships.

johnclary · 2024-12-20T16:48:42Z

app/utils/graphql.ts

-            label
+          ${typename}(order_by: {${relationship.labelColumnName}: asc}) {
+            id: ${relationship.idColumnName}
+            label: ${relationship.labelColumnName}


like i mention in the JSdoc for this hook—we are aliasing the id and label properties of this query builder, so that any lookup table we load will have the same shape. right now, we're only using lookups in the lookups schema in our db, which all of the same shape, but if we wanted to get lookup values to an EMS record, we could do that as well and still use the same UI form components.

johnclary · 2024-12-20T16:51:14Z

app/components/DataCard.tsx

just want to mention this right at the top—i want to replace the word Column, Col, etc everywhere with the term field. that just feels more intuitive and accurate. would it be worth doing that in a follow-up PR?

keeping it separated in a different PR would be good, so its not confused with any other enhancements in a PR (basically so I am not confused)

+1 - i think that a separate PR would be good because this one took some time for me to read through as-is - lots of learning! 🙏

johnclary · 2024-12-20T17:00:35Z

app/configs/chargeColumns.tsx

i am also looking for feedback on the naming conventions and file organization around these configs. we have a few different kinds of configs:

configs that organize column metadata (the ColDataCardDef<T> objects)

configs that consume the column configs ☝️ and define which UI components render which columns (cards and related tables)

the queryConfig objects that plug into the graphql table component for our crashes and locations list views

after looking at this code, part of me wondered what it would look like to organize the config folder into subfolders. but then we would have to decide on how to organize - by type like table, columns, or cards or by record type like crash, unit, location... but neither feels like a good fit right now.

So, I like the current config organization, and I think the objects themselves + the file names make it straightforward to understand where they are used and how to update them to add/update fields or change behavior. I could imagine someone finding the components by route first and then following that path to the configs. curious about other thoughts too!

i think that's a great idea. i added a todo for this 🙏

mddilley

This looks great, and I learned a lot. I like the new abstraction too! The location table shows the statistics just as expected, but I ran into one question about the light condition field in the crash view that I wasn't sure about. Other than that, it looks 🚢!

mddilley · 2024-12-27T19:25:49Z

app/components/DataCard.tsx

+    // Save the value to the foreign key column, if exists
+    const saveColumnName = editColumn.relationship?.foreignKey
+      ? editColumn.relationship?.foreignKey
+      : editColumn.path;


thanks for pointing this out. i think this part of apps always gets a bit wrinkly, and I don't think that it is too bad to not have this documented and maybe even a bit intuitive? i also have not written any of this code so I could be wrong!

mddilley · 2024-12-27T19:40:18Z

app/configs/chargeColumns.tsx

    label: "Citation Number",
  },
-};
+} satisfies Record<string, ColDataCardDef<Charge>>;


thanks for pointing to the docs for this one, and I agree that it fits this nicely. I played around with this in my editor, and I see the bug replicated when I go back to the previous typing on this config. sad about no pizza 🍕🥲 but happy about the TIL!

mddilley · 2024-12-27T19:50:31Z

app/configs/crashesColumns.tsx

      tableSchema: "lookups",
      tableName: "collsn",
+      idColumnName: "id",
+      labelColumnName: "label",
+      foreignKey: "fhe_collsn_id",


this is so slick! love that the path notation feels like lodash .get() notation which i think is a good sign for this being intuitive. 🚀

app/configs/crashesColumns.tsx

mddilley · 2024-12-27T20:14:13Z

app/types/types.ts

  sortable?: boolean;
-  valueGetter?: (record: T, column: ColDataCardDef<T>) => any;


cool to see that this is no longer needed too!

mddilley · 2024-12-27T20:23:42Z

app/components/DataCard.tsx

+1 - i think that a separate PR would be good because this one took some time for me to read through as-is - lots of learning! 🙏

mddilley · 2024-12-27T20:26:29Z

app/types/utils.ts

+ * For example, given a `<Person>` object like `{ info : { name: { first: "john" } } }`,
+ * `"info.name.first"` is a valid `Path<Person>`.
+ * 
+ * todo: use a 3rd party solution? 


i think a third-party could be good down the line if we need to support more complex shapes, but I am appreciating having this code here to learn from right now. 📚 This is pretty heady for me right now, but I think I'm getting there!

mddilley · 2024-12-27T20:27:27Z

app/utils/formHelpers.ts

+ * Retrieve a value from an object given a dot-noted path string.
+ *
+ * Essentially, it performs optional chaining on an object. It is similar
+ * to lodash.get(), except array notation is not supported.


ah cool! lodash was the inspiration. ✨

mddilley · 2024-12-27T20:58:05Z

app/configs/chargeColumns.tsx

after looking at this code, part of me wondered what it would look like to organize the config folder into subfolders. but then we would have to decide on how to organize - by type like table, columns, or cards or by record type like crash, unit, location... but neither feels like a good fit right now.

So, I like the current config organization, and I think the objects themselves + the file names make it straightforward to understand where they are used and how to update them to add/update fields or change behavior. I could imagine someone finding the components by route first and then following that path to the configs. curious about other thoughts too!

mateoclarke

🚢 Lots of interesting Typescript goodness happening and I think I'm following the direction to use "paths" and "relationships" to have more dynamic attribute stringing with dot-notation. Thanks for adding commentary and links. ✨

johnclary added 3 commits December 17, 2024 10:50

introduce property + type and use it everywhere

61e31e4

lint

f14c8eb

simplify typing on getFromPath

1f6cb50

wip remove renderLookupLabel

b74551d

johnclary changed the title ~~Add support for rendering nested record properties~~ Add summary statistics to location details Dec 19, 2024

johnclary added the WIP Work in progress label Dec 19, 2024

johnclary added 8 commits December 19, 2024 13:12

make card width larger on md+

a655045

add location stats and cost formatter

37ce3e6

lint

7386d24

rework column 'relationship' prop and make edit components to use it

b5b4150

remove todo

eb53433

Merge branch 'nextjs' into john/20316-nested-object-support

6cf2a79

update city and charges to follow new schema

5f034cf

uses ts satisfies to add strong typing to configs

7b75854

johnclary changed the title ~~Add summary statistics to location details~~ Add summary statistics to location details and rework config types Dec 20, 2024

johnclary changed the title ~~Add summary statistics to location details and rework config types~~ Add summary statistics to location details and rework column metadata types Dec 20, 2024

johnclary commented Dec 20, 2024

View reviewed changes

use satisfies on loc cols

36b9a3b

johnclary commented Dec 20, 2024

View reviewed changes

johnclary requested review from chiaberry, frankhereford, mateoclarke, tillyw, mddilley and roseeichelmann December 20, 2024 16:52

johnclary commented Dec 20, 2024

View reviewed changes

johnclary removed the WIP Work in progress label Dec 20, 2024

Merge branch 'nextjs' into john/20316-nested-object-support

2195d11

johnclary mentioned this pull request Dec 23, 2024

Fatalities listing page #1637

Open

4 tasks

mddilley reviewed Dec 27, 2024

View reviewed changes

mateoclarke approved these changes Dec 31, 2024

View reviewed changes

johnclary mentioned this pull request Dec 31, 2024

Crash details: people related records table #1642

Open

4 tasks

mddilley approved these changes Dec 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add summary statistics to location details and rework column metadata types #1634

Add summary statistics to location details and rework column metadata types #1634

johnclary commented Dec 18, 2024 •

edited

Loading

netlify bot commented Dec 18, 2024 •

edited

Loading

johnclary Dec 20, 2024 •

edited

Loading

johnclary Dec 20, 2024

johnclary Dec 20, 2024 •

edited

Loading

johnclary Dec 20, 2024 •

edited

Loading

mddilley Dec 27, 2024

johnclary Dec 20, 2024

johnclary Dec 20, 2024 •

edited

Loading

mddilley Dec 27, 2024

chiaberry Dec 31, 2024

johnclary Dec 20, 2024

johnclary Dec 20, 2024

mddilley Dec 27, 2024

johnclary Dec 20, 2024 •

edited

Loading

johnclary Dec 20, 2024

johnclary Dec 20, 2024

johnclary Dec 20, 2024

johnclary Dec 20, 2024

johnclary Dec 20, 2024

chiaberry Dec 26, 2024

mddilley Dec 27, 2024

johnclary Dec 20, 2024

mddilley Dec 27, 2024

johnclary Dec 29, 2024

mddilley left a comment

mddilley Dec 27, 2024

mddilley Dec 27, 2024

mddilley Dec 27, 2024

mddilley Dec 27, 2024

mddilley Dec 27, 2024

mddilley Dec 27, 2024 •

edited

Loading

mddilley Dec 27, 2024

mddilley Dec 27, 2024

mateoclarke left a comment

		sortable?: boolean;
		valueGetter?: (record: T, column: ColDataCardDef<T>) => any;

Add summary statistics to location details and rework column metadata types #1634

Are you sure you want to change the base?

Add summary statistics to location details and rework column metadata types #1634

Conversation

johnclary commented Dec 18, 2024 • edited Loading

Associated issues

Testing

Ship list

netlify bot commented Dec 18, 2024 • edited Loading

✅ Deploy Preview for vision-zero-nextjs ready!

johnclary Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnclary Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

johnclary Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnclary Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnclary Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mddilley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mddilley Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateoclarke left a comment

Choose a reason for hiding this comment

johnclary commented Dec 18, 2024 •

edited

Loading

netlify bot commented Dec 18, 2024 •

edited

Loading

johnclary Dec 20, 2024 •

edited

Loading

johnclary Dec 20, 2024 •

edited

Loading

johnclary Dec 20, 2024 •

edited

Loading

johnclary Dec 20, 2024 •

edited

Loading

johnclary Dec 20, 2024 •

edited

Loading

mddilley Dec 27, 2024 •

edited

Loading