Fix time validation, standardize request modals#609
Conversation
|
[diff-counting] Significant lines: 189. |
There was a problem hiding this comment.
I'm a little iffy about this change, because as someone with a certified trash memory ™ I'm the kind of person that would need to press the submit button multiple times to remember what I did wrong because the error text doesn't persist 😭. Still an understandable change for various benefits, but maybe some discussion about the tradeoffs is needed
| const [updating, setUpdating] = useState(false); | ||
| const [saving, setSaving] = useState(false); | ||
| const [timeErrorOpen, setTimeErrorOpen] = useState(false); | ||
| const [timeErrors, setTimeErrors] = useState<TimeValidationError[]>([]); |
There was a problem hiding this comment.
Do timeErrorOpen and timeErrors need to be independent pieces of state? Can one be derived with a useMemo?
| { | ||
| allowPastTimes, | ||
| maxDurationHours: 24, | ||
| minDurationMinutes: 5, |
There was a problem hiding this comment.
How was 5 minutes chosen as the minimum? We might want to just make this 0 or 1 so we never get in the way on minimums.
| </DialogActions> | ||
| </Dialog> | ||
|
|
||
| {/* Time Validation Errors Modal */} |
There was a problem hiding this comment.
I might have missed when this was already discussed, but what was the reasoning behind a modal specifically for these validation errors? Wouldn't something inline on the layout be a little more clear, to avoid stacking modals on top of modals, or are we switching due to layout shift and consistency issues? Either way it might be useful if the PR summary had justification behind this consideration.
| editedRide.endTime, | ||
| { | ||
| allowPastTimes: !isNewRide(editedRide), | ||
| allowPastTimes: false, |
There was a problem hiding this comment.
Am I misreading something, or does this change mean past rides can never be edited? Does this effectively freeze past rides in place?
|
Outdated |
3 participants
Summary
Fixed next/current ride edit button and ride table on-click function to lead to the same ride detail/edit modal; added consistent time validation when editing rides.
Test Plan
Tested through rigorous edge case input checking; anyone reviewing this PR should definitely do some of that on their own too to verify correctness.