Add summary statistics to location details and rework column metadata types #1634
There are no files selected for viewing
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. 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) There was a problem hiding this comment. +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! 🙏 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,7 @@ import { | |
| handleFormValueOutput, | ||
| } from "@/utils/formHelpers"; | ||
| import { ColDataCardDef } from "@/types/types"; | ||
| import { LookupTableOption } from "@/types/relationships"; | ||
|
|
||
| interface DataCardProps<T extends Record<string, unknown>> { | ||
| record: T; | ||
|
|
@@ -37,7 +37,12 @@ export default function DataCard<T extends Record<string, unknown>>({ | |
| // todo: handling of null/undefined values in select input | ||
| const [editColumn, setEditColumn] = useState<ColDataCardDef<T> | null>(null); | ||
| const { mutate, loading: isMutating } = useMutation(mutation); | ||
| const [query, typename] = useLookupQuery( | ||
| editColumn?.editable && editColumn?.relationship | ||
| ? editColumn.relationship | ||
| : undefined | ||
| ); | ||
|
There was a problem hiding this comment. we only need to use the lookup query when a column is editable and it has relationship metadata |
||
|
|
||
| const { data: selectOptions, isLoading: isLoadingLookups } = | ||
| useQuery<LookupTableOption>({ | ||
| query, | ||
|
|
@@ -47,10 +52,18 @@ export default function DataCard<T extends Record<string, unknown>>({ | |
| }); | ||
|
|
||
| const onSave = async (value: unknown) => { | ||
| if (!editColumn) { | ||
| // not possible | ||
| return; | ||
| } | ||
| // Save the value to the foreign key column, if exists | ||
| const saveColumnName = editColumn.relationship?.foreignKey | ||
| ? editColumn.relationship?.foreignKey | ||
| : editColumn.path; | ||
|
There was a problem hiding this comment. this hints at the awkward dance we need to do between rendering a value from an object relationship. E.g. This PR is trying to iron this out by moving the necessary metadata into the column 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. There was a problem hiding this comment. 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 # 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 There was a problem hiding this comment. 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! |
||
| await mutate({ | ||
| id: record.id, | ||
| updates: { | ||
| [saveColumnName]: value, | ||
| }, | ||
| }); | ||
| await onSaveCallback(); | ||
|
|
@@ -66,10 +79,10 @@ export default function DataCard<T extends Record<string, unknown>>({ | |
| <Table responsive hover> | ||
| <tbody> | ||
| {columns.map((col) => { | ||
| const isEditingThisColumn = col.path === editColumn?.path; | ||
| return ( | ||
| <tr | ||
| key={String(col.path)} | ||
| style={{ | ||
| cursor: | ||
| col.editable && !isEditingThisColumn ? "pointer" : "auto", | ||
|
|
@@ -95,14 +108,14 @@ export default function DataCard<T extends Record<string, unknown>>({ | |
| {!isLoadingLookups && ( | ||
| <DataCardInput | ||
| initialValue={valueToString( | ||
| getRecordValue(record, col, true), | ||
| col | ||
| )} | ||
| onSave={(value: string) => | ||
| onSave( | ||
| handleFormValueOutput( | ||
| value, | ||
| !!col.relationship, | ||
| col.inputType | ||
| ) | ||
| ) | ||
|
|
||
|
There was a problem hiding this comment. i've mentioned this elsewhere, but it is rather unfortunate that the |
|
There was a problem hiding this comment. i am also looking for feedback on the naming conventions and file organization around these configs. we have a few different kinds of configs:
There was a problem hiding this comment. after looking at this code, part of me wondered what it would look like to organize the 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! There was a problem hiding this comment. i think that's a great idea. i added a todo for this 🙏 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,21 +1,21 @@ | ||
| import { ColDataCardDef } from "@/types/types"; | ||
| import { Charge } from "@/types/charge"; | ||
|
|
||
| export const ALL_CHARGE_COLUMNS = { | ||
| unit_nbr: { | ||
| path: "unit_nbr", | ||
| label: "Unit", | ||
| }, | ||
| prsn_nbr: { | ||
| path: "prsn_nbr", | ||
| label: "Person", | ||
| }, | ||
| charge: { | ||
| path: "charge", | ||
| label: "Charge", | ||
| }, | ||
| citation_nbr: { | ||
| path: "citation_nbr", | ||
| label: "Citation Number", | ||
| }, | ||
| } satisfies Record<string, ColDataCardDef<Charge>>; | ||
|
There was a problem hiding this comment. 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 this change fixes a type bug down in the card/table configurations 👇 There was a problem hiding this comment. 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! There was a problem hiding this comment. I liked this blog post about satisfies: https://fnune.com/typescript/2022/11/15/typescript-series-5-record-and-the-satisfies-operator/ |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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