WIP: Custom Location Enhancements#629
Conversation
…in depth matchingfor checking if a custom locaiton is already in the database
|
[diff-counting] Significant lines: 125. |
benkoppe
left a comment
benkoppe
left a comment
There was a problem hiding this comment.
Good work! Is this PR still in WIP because the changes need to be standardized to the admin version of the modal?
| if (selectionState === 'pickup') { | ||
| // Show all locations for pickup selection | ||
| return supportLocsWithOther; | ||
| return supportLocsWithOther; |
There was a problem hiding this comment.
I think this duplicate line might be a mistake.
| setConfirmDialogOpen(false); | ||
| }; | ||
|
|
||
| /** |
There was a problem hiding this comment.
There might have been some git merge issues in this file. There are some duplicate line additions, such as this comment!
| const handleDateChange = | ||
| (field: keyof Pick<FormData, 'date' | 'time' | 'repeatEndDate'>) => | ||
| (newDate: Date | null) => { | ||
| if (field === 'time') { |
There was a problem hiding this comment.
Would it be better to separate time changes into a separate function from date changes?
| const isValid = | ||
| newDate !== null | ||
| ? isValidTime(newDate) | ||
| : setFormData({ |
There was a problem hiding this comment.
I'm confused by the flow of this function. If working with a time field, does this mean you call setFormData from within the ternary statement, and then also call setFormData again lower down? I think there may be a better way of doing things.
| }); | ||
| }; | ||
|
|
||
| const isValidTime = (time: Date) => { |
There was a problem hiding this comment.
This is a useful function but it might be better suited for some sort of shared utility file, especially since it enforces rules that we will probably want to keep in-sync across the codebase.
|
|
||
| useEffect(() => { | ||
| if (overlayRadius) { | ||
| setKMLLink( |
There was a problem hiding this comment.
I love how you integrated KML files! But is this the best place to store the constant value of the link? We probably don't want to hard-code it in a useEffect.
Additionally, useEffect probably isn't needed here at all. Because what you're doing here is effectively making kmlLink a derived state of overlayRadius -- if it's false, the link is null, if it's true, the link is set to the constant string. A useMemo is always better in this case (fully derived state w/ no side effects).
| <InputLabel>Pickup Location</InputLabel> | ||
| <Select<string> | ||
| value={formData.pickupLocation?.id || ''} | ||
| onChange={(event) => { |
There was a problem hiding this comment.
Definitely consider extracting this to a function, it's a lot of code to embed in the layout.
| <Select<string> | ||
| value={formData.dropoffLocation?.id || ''} | ||
| disabled={!formData.pickupLocation} //makes ensuring start and end locations are different simpler | ||
| onChange={(event) => { |
There was a problem hiding this comment.
Like the above comment, I think this might also be better as a separate function.
3 participants
Summary
This pull request is the first step towards implementing feature Foo
Test Plan
Notes
The KML link is not CU-Lift's actual radius right now because I did not have the proper permissions to export the file. Also, I plan on adding location validation in this PR that checks whether or not a custom location is actually allowed
Breaking Changes