-
Notifications
You must be signed in to change notification settings - Fork 4
Fatality details page: Create summary/map card #1880
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/create_fatality_page_24771
Are you sure you want to change the base?
Fatality details page: Create summary/map card #1880
Conversation
❌ Deploy Preview for atd-vze-staging failed. Why did it fail? →
|
editor/utils/map.ts
Outdated
| ? mapStyleOptions.darkStreets | ||
| : mapStyleOptions.lightStreets; | ||
| if (useColorStreets) { | ||
| return mapStyleOptions.colorStreets; |
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.
adding the third option of using the colorful streets basemap instead of light vs dark
also -- im not fuly convinced that we shouldnt just make the colorful street map what you see on light mode? so the crashes list page would should the colorful streetmap on light mode too, and dark mode would still show the dark street map on all the maps. 🤷 @johnclary thoughts?
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 am in total agreement, and actually wrote this into the issue 😎
As part of the this work—replace the light/dark basemaps with the color street map (streets-v12) on the Location details, crash details, and EMS details pages as well
I wasn't sure about keeping the dark basemap, but I am convinced now that it's the way to go. So yeah, what you said!
Edit: ah, i see i left out the list pages from the scope. I think we should get ride of the light basemap everywhere 👍
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.
okay i went ahead and updated the light mode streets basemap to use the colorful one for all maps 💯
| if (!value || typeof value !== "string") { | ||
| return ""; | ||
| } | ||
| return format(parseISO(value), "MM/dd/yyyy h:mm a — E") || ""; |
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 marys preferred formatting with the slashes //'s ? it strays from our other date formatting and i remember once xavier talking about streamlining all or date formatting 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.
Yes, it is. Thanks for flagging it. We have at least 3 different ways of formatting date + time in the app right now. I'd like to get down to just one standard way before we ship this. I'll scope that!
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.
This looks great, Rose! I guess I'll hold my approval if you're going to replace the light maps from the list views.
| : "TxDOT"} | ||
| </td> | ||
| </tr> | ||
| </tbody> |
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 looks good to me. If we need to repeat this pattern again, I think we should extract a DataCardBody component from the DataCard component and drop that in as a replacement.
| if (!value || typeof value !== "string") { | ||
| return ""; | ||
| } | ||
| return format(parseISO(value), "MM/dd/yyyy h:mm a — E") || ""; |
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.
Yes, it is. Thanks for flagging it. We have at least 3 different ways of formatting date + time in the app right now. I'd like to get down to just one standard way before we ship this. I'll scope that!
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.
Its coming together! I like the colorful basemap
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.
Love seeing this view getting built out! I flagged one issue with the roadway owner field, but this all looks great. The basemap update looks ✨! 🎨
| Roadway owner | ||
| </td> | ||
| <td> | ||
| {crash.is_coa_roadway === 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.
@roseeichelmann I couldn't find a crash that showed City of Austin, and I think it is because it is missing from the query and response (and maybe permissions?). Code to handle looks great!
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 catch Mike!
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!! usually there is an error when you try to access a property that doesnt exist... i wonder why that didnt happen? is it bc the type for Crash.is_coa_roadway is boolean OR null?
…field Add units types involved view
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 looks great Rose, thank you! I think your change of using the colorful map in light mode across the board is a big level up - looks fantastic!
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.
Retested with the latest update, and I can the corresponding roadway owners when looking at on- or off-system records. 🚢 😎
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.
looks good! ✅
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.
👍
Associated issues
Closes cityofaustin/atd-data-tech#24972
Testing
URL to test:
Local
Steps to test:
Ship list
mainbranch