Ability to upload and delete crash diagrams for temporary records#1971
Ability to upload and delete crash diagrams for temporary records#1971roseeichelmann wants to merge 10 commits intomainfrom
Conversation
✅ Deploy Preview for atd-vze-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
I ended up DRYing it up and making this into a reuseable/interchangeable component for both image upload modals (fatalities and crash diagrams)
| </Form.Control.Feedback> | ||
| </Form.Group> | ||
| </Col> | ||
| {needsImageSource && ( |
There was a problem hiding this comment.
this whole change is just checking if this modal needs the image source field (such as in the fatality upload modal)
|
|
||
| fetchImage(); | ||
| }, [personId, getToken, imageVersion]); | ||
| const { imageUrl, refetch, isLoading } = useImage({ |
There was a problem hiding this comment.
wow i did not know about this useImage hook @johnclary made 🤦 got to really DRY up all this code by deleting my useEffect above that is basically an exact replica of this useImage hook thats already being used for the crash diagrams
There was a problem hiding this comment.
oh nice! i forgot about it even though it was just a few weeks ago: #1959 (comment)
| setShowModal={setShowModal} | ||
| storedUrl={imageUrl} | ||
| title={`Upload crash diagram`} | ||
| url={`${process.env.NEXT_PUBLIC_CR3_API_DOMAIN}/images/crash_diagram/${crash.record_locator}`} |
There was a problem hiding this comment.
if you fudge up this URL in your code editor you can test the error message in the modal
| const token = await getToken(); | ||
| const response = await fetch(url, { | ||
| headers: { | ||
| Authorization: `Bearer ${token}`, |
There was a problem hiding this comment.
remove this line in your code editor to test error displayed when token is missing
johnclary
left a comment
There was a problem hiding this comment.
Wow—works great and looks great. And coming at at just +13 lines of code 🙌
I left really minor feedback. I tested this quite a lot and everything is super smooth 🚀
| import Image from "react-bootstrap/Image"; | ||
| import Button from "react-bootstrap/Button"; | ||
| import { FaRotate } from "react-icons/fa6"; | ||
| import { FaRegPenToSquare } from "react-icons/fa6"; |
There was a problem hiding this comment.
Would you switch this to
import { LuSquarePen } from "react-icons/lu";
😇
| setShowModal={setShowModal} | ||
| storedUrl={imageUrl} | ||
| title={`Upload crash diagram`} | ||
| url={`${process.env.NEXT_PUBLIC_CR3_API_DOMAIN}/images/crash_diagram/${crash.record_locator}`} |
|
|
||
| fetchImage(); | ||
| }, [personId, getToken, imageVersion]); | ||
| const { imageUrl, refetch, isLoading } = useImage({ |
There was a problem hiding this comment.
oh nice! i forgot about it even though it was just a few weeks ago: #1959 (comment)
| const file = watch("file"); | ||
|
|
||
| const url = `${process.env.NEXT_PUBLIC_CR3_API_DOMAIN}/images/person/${personId}`; | ||
| const needsImageSource = defaultValues.hasOwnProperty("image_source"); |
There was a problem hiding this comment.
Love that you are making this component reusable!
This is working fine, but I think it could be more obvious from the component API that there is a switch that toggles the image source requirement.
Even thought it makes the component less abstract, it might be be better to remove the url and defaultValues props and instead use an explicit prop like imageType that indicates if the image is a crash diagram or person image. And then inside the component you can supply the correct url and defaultValues based on this prop value.
That way the dev doesn't have to know the right combination of URL and default values because the component handles it for you.
chiaberry
left a comment
There was a problem hiding this comment.
Tested uploading and deleting!
I kind of want a "Is temp crash" filter, but I just searched by T in the search bar and they came up.
What do you think about rewording the message "Crash diagrams are not available for temporary crash records". Seems a little contradictory saying that when theres a button immediately below it to add one.
I see John's comment that you are going to rework some of the component, so I'll do a quick retest after that and approve then.
|
@johnclary ill learn to scroll someday |

Associated issues
Closes cityofaustin/atd-data-tech#26662
Testing
URL to test:
Local
Steps to test:
./vision-zero local-stack-upShip list
mainbranch