-
Notifications
You must be signed in to change notification settings - Fork 4
Fatality details page: create Victims card #1892
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
base: rose/fatality_page_summary_24972
Are you sure you want to change the base?
Fatality details page: create Victims card #1892
Conversation
| </span> | ||
| {victim.prsn_type_id !== 3 && // pedalcyclist | ||
| // Dont show person type for cyclists or pedestrians bc its redundant | ||
| victim.prsn_type_id !== 4 && ( // pedestrian |
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.
hardcoding ids makes me nervous bc will CRIS ever change this? i think this is the best option though 🤷
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.
Agreed that there's nothing we can do about future CRIS breaking changes. One thing that could help future devs is to isolate the magic numbers outside of the render code. E.g. in an isPedestrian() and isCar() function 🤷
| // Only show restraint field for cars | ||
| unit.unit_desc_id === 1 && // motor vehicle | ||
| unit.veh_body_styl_id !== 71 && // motorycle | ||
| unit.veh_body_styl_id !== 90 && ( // police motorcycle |
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.
hardcoding IDs again
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.
thanks for including the type in comments next to the hardcoded IDs.
tiny typo in motorcycle for id 71
johnclary
left a comment
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.
Rose, this is looking good. I am requesting changes for the person age/ethnicity/sex formatting.
| </span> | ||
| {victim.prsn_type_id !== 3 && // pedalcyclist | ||
| // Dont show person type for cyclists or pedestrians bc its redundant | ||
| victim.prsn_type_id !== 4 && ( // pedestrian |
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.
Agreed that there's nothing we can do about future CRIS breaking changes. One thing that could help future devs is to isolate the magic numbers outside of the render code. E.g. in an isPedestrian() and isCar() function 🤷
| </div> | ||
| <div className="mb-1 d-flex w-100 flex-column"> | ||
| <span className="mb-2"> | ||
| {victim.prsn_age} YEARS OLD -{" "} |
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.
Looks like we need some handling for when any of age/sex/ethnicity are missing. I think we can exclude any component when it is null, "UNKNOWN", or "AUTONOMOUS".
What do you think about this? 👇
- Should be
"HISPANIC MALE". I'm not sure if we want to default to anUNKNOWNname in this case. I assume we will leave it up to VZ team to do so, but I'll check with them.
- This should be
"MALE", with"YEARS OLD - UNKNOWN"hidden
- This should be
"33 YEARS OLD - MALE"
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.
Why exclude people categorized as "Other" ethnicities?
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 guess I think "OTHER MALE" and "OTHER FEMALE" is an odd way to describe someone vs not mentioning their ethnicity at all.
Tbh, I think my preference would be that we only show victim age and not sex and ethnicity. The CRIS/EMS ethnicity disagree very, very often (esp hispanic/white/asian), and listing "male" and "female" reads as clinical and dehumanizing to me 😕
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 only showing age and not sex and ethnicity especially since I believe ethnicity is not self reported and is what the person filling out the form is saying.
I agree that OTHER FEMALE is odd, one way is to put commas between the identifiers, WHITE, MALE. I questioned ignoring OTHER because it's not that it's necessarily unknown (though I'd guess often it is) and it feels off to me to exclude mention of an ethnicity just because it doesn't fit into the arbitrary 4 main ones CRIS has chosen.
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 agree w yall that the current formatting feels off for showing "Other" or "Unknown". I will say that while i get what john is saying about it being clinical, not including sex or ethnicity on the page at all feels like obfuscating information that we already show in other parts of the app and i think mary expects to see those fields here?
I think if we keep the sex/ethnicity identifiers we should maybe put them each on their own line or something and prepend w the label so like --
Ethnicity: Other
Sex: Male
Ethnicity: Unknown
Sex: Unknown
I think w the above formatting we should still show Unknown and Other bc they still provide some info
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.
Thanks for that feedback. I messaged Mary to see how she currently handles when these descriptors are unknown.
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.
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 dont mind the separate lines in ur lil mockup but im down to circle back for polishing later... im still of the mind that we should show as much information as possible, including "Other" for ethnicity
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'm way late here but i agree that separate lines might not be so bad, but I love that y'all are looking for feedback from Mary. I've really enjoyed y'all's discussion about being able to replace some of the prep work of the monthly reporting with this view. ❤️
| drvr_city_name: string; | ||
| drvr_ethncty: LookupTableOption; | ||
| prsn_ethnicity_id: number; | ||
| drvr_ethncty?: LookupTableOption; |
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.
Glad to see you catching this typeError waiting to happen!
I haven't written an issue for this, but I'd really like to go through all our types and make every property that comes from Hasura optional (?) 😅
| // Only show restraint field for cars | ||
| unit.unit_desc_id === 1 && // motor vehicle | ||
| unit.veh_body_styl_id !== 71 && // motorycle | ||
| unit.veh_body_styl_id !== 90 && ( // police motorcycle |
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.
thanks for including the type in comments next to the hardcoded IDs.
tiny typo in motorcycle for id 71
| </div> | ||
| <div className="mb-1 d-flex w-100 flex-column"> | ||
| <span className="mb-2"> | ||
| {victim.prsn_age} YEARS OLD -{" "} |
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.
Why exclude people categorized as "Other" ethnicities?
| {victim.injry_sev?.label && ( | ||
| <span | ||
| className={`${getInjuryColorClass(victim.injry_sev.label)} d-inline-flex align-items-center justify-content-center px-2 py-1 rounded `} | ||
| style={{ minWidth: "fit-content" }} | ||
| > | ||
| <small>{victim.injry_sev?.label}</small> | ||
| </span> | ||
| )} |
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.
will this card ever expand to include other injury severities? If not, wondering why include the severity label here
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.
Yeah, theres a future enhancement to have an All people toggle or something like that, that will show all people that were in the crash
frankhereford
left a comment
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.
This is great Rose -- I have just two tiny layout observations, but otherwise, this implements the spec so well. Clean code, runs good! 🚢 🚢 🚢
| const unitYearMakeModel = | ||
| `${unit.veh_mod_year || ""} ${unit.veh_make?.label || ""} ${unit.veh_mod?.label || ""}`.trim(); | ||
| return ( | ||
| <ListGroupItem key={unit.id}> |
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.
It's not this line of code in particular, but the content contained below this element -- I think the left alignment can be adjusted to line these up to match the spec. Maybe the middle line can be nudged up to be a little bit closer to the first line based on an eyeball of the image in the issue too. I know this isn't very clear - LMK if I can help explain myself better. Thank you!
BTW - if you look at the image in full rez, there is a little red ruler line to help illustrate what i'm trying to say.
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.
Thanks, Frank, for giving this the mag treatment 🙏
Our current expectation (pending feedback from Mary) is that the name field never be null, or we establish a default text to place in its position, so at the moment it's ok that the DRIVER OF ... text is slightly indented by the margin of the empty victim name <span>.
Beyond that, I think we have a missing ListGroup wrapper for the victim names—I'll comment on that below. I'll double check for extra spacing sneaking in but that should make the indentation consistent with our design system.
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 kind of like the way i have each victim indented to the right under the unit and did that on purpose bc i think it helps grok all the text, esp when theres multiple people and units. but i can add that missing ListGroup wrapper if you would prefer it to look like the mockup, now im not sure what i like more 🤔
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.
Just stepping back to look at this file holistically, Rose -- it's just real fine. Super smart and reasonable component boundary, clean interface in terms of props and the typing, and your mastery over bootstrap styling and general coding legibility are really shining.
I have some comments below that feel very much little nit-picky things, and as I submit those to you, I wanted to also acknowledge that they are tiny nothings in the big picture of your work's very high quality.
| <div className="d-flex align-items-center"> | ||
| <span className="fw-bold me-2"> | ||
| {victim.prsn_first_name} {victim.prsn_mid_name}{" "} | ||
| {victim.prsn_last_name} |
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.
would you add some conditional formatting to make sure we don't have empty spaces filling if any of the name parts are missing? I dunno if it makes sense to reuse this formatter function.
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.
do you mean if an empty string is saved in there? bc i tested w null name parts and it doesnt create empty spaces 🤷
| const unitYearMakeModel = | ||
| `${unit.veh_mod_year || ""} ${unit.veh_make?.label || ""} ${unit.veh_mod?.label || ""}`.trim(); | ||
| return ( | ||
| <ListGroupItem key={unit.id}> |
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.
Thanks, Frank, for giving this the mag treatment 🙏
Our current expectation (pending feedback from Mary) is that the name field never be null, or we establish a default text to place in its position, so at the moment it's ok that the DRIVER OF ... text is slightly indented by the margin of the empty victim name <span>.
Beyond that, I think we have a missing ListGroup wrapper for the victim names—I'll comment on that below. I'll double check for extra spacing sneaking in but that should make the indentation consistent with our design system.
| </span> | ||
| </div> | ||
| </div> | ||
| {unitVictims?.map((victim) => ( |
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.
Since we're starting a new list here, I think we want to wrap the vicitm list items in a <ListGroup variant="flush">. I'm not seeing any noticeable visible change from it so it's just semantics.
mateoclarke
left a comment
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.
The example crash you linked with 5 fatalities so very depressing. But the purpose of this Victims view is to humanize these individuals. All the thoughtful convo about how to do that with limited and inaccurate data is refreshing. Thanks for leading this effort Rose!
mddilley
left a comment
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.
Tested locally, and everything looks as described in the test steps. This page is going to help the VZ team represent these people and their stories so much. ❤️

Associated issues
Closes cityofaustin/atd-data-tech#24973
Testing
URL to test:
Local
Steps to test:
Ship list
mainbranch