-
Notifications
You must be signed in to change notification settings - Fork 4
Redesigned victims section with unit cards, contributing factors, and charges #1917
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/narrative_summary_24776
Are you sure you want to change the base?
Redesigned victims section with unit cards, contributing factors, and charges #1917
Conversation
| import { MapRef } from "react-map-gl"; | ||
| import { PointMap } from "@/components/PointMap"; | ||
| import FatalityVictimsCard from "@/components/FatalityVictimsCard"; | ||
| import FatalityUnitsCards from "@/components/FatalityUnitsCards"; |
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.
renamed for clarity
| <Image | ||
| alt="placeholder" | ||
| className="me-3" | ||
| src={`${BASE_PATH}/assets/img/avatars/placeholder.png`} |
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.
is this placeholder image fine? it was already in our assets
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 looks good 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.
Me too—thanks for this!
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 was just some linting of some sort
| <h5 className="mb-1 me-2"> | ||
| <span className="fw-bold"> Unit {unit.unit_nbr}</span> | ||
| </h5> | ||
| <div className="d-flex flex-grow-1 justify-content-start"> |
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.
original mockups have this unit year make & model in the text-secondary class, but i found that the contrast was way below accessibility standards, especially for dark mode:
In light mode its not as bad:
and you can compare with what i have now in this PR for light/dark mode just using the primary text color. thoughts? i recognize the DRIVER is still in that secondary color but the contrast isnt as bad w the background color
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.
Using the primary text color is great—thanks for that!
And, yes, rendering our secondary color over white background should be fine for color contrast.
chiaberry
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.
I left a few comments about duplicate key and the person experiencing homelessness section, but overall this looks great!
| {victim.prsn_exp_homelessness && ( | ||
| <span>{victim.prsn_exp_homelessness}</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.
This is not rendering anything because its just resolving to an empty span when True
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.
thank you for catching this
| <div className="pb-1"> | ||
| <div className="fw-bold">Charges</div> | ||
| {unitCharges.map((charge) => ( | ||
| <div className="ms-2" key={charge.citation_nbr}> |
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 got a "same key error" in the console when viewing the crash you linked in the summary, the same citation_nbr was on two charges
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.
🙏
| <Image | ||
| alt="placeholder" | ||
| className="me-3" | ||
| src={`${BASE_PATH}/assets/img/avatars/placeholder.png`} |
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 looks good to me!
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 looks really good. I've dropped in a few comments and suggestions but this is looking pretty much ready to ship 🙌
|
|
||
| const charges = crash.charges_cris; | ||
|
|
||
| const BASE_PATH = process.env.NEXT_PUBLIC_BASE_PATH || ""; |
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 can be declared outside the component
| // Show a card for units with fatalities, charges, or contrib factors | ||
| if (hasVictim || hasCharges || hasContribFactors) { | ||
| const unitYearMakeModel = | ||
| `${unit.veh_mod_year || ""} ${unit.veh_make?.label || ""} ${unit.veh_mod?.label || ""}`.trim(); |
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.
For readability, it might be nice to move all this data munging into a helper function
| // Get the list of fatalities in the unit | ||
| const unitVictims = victims?.filter( | ||
| (victim) => victim.unit_nbr === unit.unit_nbr | ||
| ); |
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 would be cool if this unit-person hierarchy came through from the graphql API. any reason not to pull the people records via the unit-person relationship instead?
| <Image | ||
| alt="placeholder" | ||
| className="me-3" | ||
| src={`${BASE_PATH}/assets/img/avatars/placeholder.png`} |
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.
Me too—thanks for this!
| <span className="pb-1"> | ||
| Restraint used: {victim.rest.label} | ||
| </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.
It would be cool to have helper functions to trim down all this render code
| </Card.Body> | ||
| )} | ||
| {(hasCharges || hasContribFactors) && ( | ||
| <Card.Footer |
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.
maybe spin this off into a UnitCardFooter component?
| valueRenderer: (record) => { | ||
| const value = record.injry_sev?.label || ""; | ||
| const className = `${getInjuryColorClass(value)} px-2 py-1 rounded`; | ||
| const className = `${getInjuryColorClass(value)} px-2 py-1 rounded text-center`; |
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.
| [data-bs-theme="light"] .fatality-units-card-header-footer { | ||
| background-color: $light !important; | ||
| } | ||
|
|
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.
🙏
| <h5 className="mb-1 me-2"> | ||
| <span className="fw-bold"> Unit {unit.unit_nbr}</span> | ||
| </h5> | ||
| <div className="d-flex flex-grow-1 justify-content-start"> |
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.
Using the primary text color is great—thanks for that!
And, yes, rendering our secondary color over white background should be fine for color contrast.
| <Col className="mb-3" sm={12} md={6}> | ||
| <FatalityVictimsCard crash={crash} /> | ||
| <FatalityUnitsCards crash={crash} /> | ||
| </Col> |
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.
What do folks think about how to handle when the unit cards vertically overflow the height of the map card? I initially thought we should set a max-height on this column and add vertical scroll so that height of both rows of cards is consistent. But I assume Mary would probably prefer to have all victims visible without scrolling. It is very rare to have a crash more than two fatalities—just three crashes in the last five years.
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.
Good question! 🤔 TLDR: I think it's cool as is, but I say that without strong conviction.
In Rose's testing instructions, she asks for the viewport to be resized to see how it reacts under different widths, and I happened to be using the crash with four fatalities. It didn't seem unnatural to me the way it is now with the whitespace below the map to give the full-height list of the 4 fatalities.
I think works as is, and if I were to change anything, I'd see if the map could be made to expand to fill the available height but only up to the height of the units + fatality cards. That might not be a great idea as I say it, but something to try?
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 works great Rose. I spun it up and I think it's 💯 . Thank you for this work! 🙏

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