Request ride workflow UI#676
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy Preview for carriage-web ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
YottaYocta
left a comment
There was a problem hiding this comment.
Thanks Emir, great work!
I left some comments for small styling changes.
I haven't gotten though all the code yet, so i'll be leaving more comments as I parse the rest of this comment.
| return ( | ||
| <div className={styles.stepPage}> | ||
| <div className={styles.topContent}> | ||
| {/* Drag handle at the top (click to close on desktop, drag down on mobile) */} | ||
| <div |
| import moment from 'moment'; | ||
| import styles from './requestridemodal.module.css'; | ||
|
|
||
| type CustomDatePickerProps = { | ||
| selectedDate: moment.Moment; | ||
| currentMonth: moment.Moment; | ||
| onMonthChange: (month: moment.Moment) => void; | ||
| onDateSelect: (date: moment.Moment) => void; |
There was a problem hiding this comment.
| availableLocations={locations} | ||
| enablePickupMapClick | ||
| onPickupSelect={handlePickupSelect} | ||
| onDropoffSelect={() => { }} |
There was a problem hiding this comment.
Encountered a bug where if i select a location, move to the next page, then move back to the original location page, it gets rid of the blue indicator for the selected location
YottaYocta
left a comment
There was a problem hiding this comment.
Hi Emir!
Thanks for the great work. Based on our convo, the main things for this PR are:
- use the MUI components for complex things like date pickers and time pickers. It's alright if they don't map 1:1 with the figma designs
- Consider lifting the state of the multi-step form. We can eliminate the wizard step, and also reduce the number of props passed between the modal parent and each step's component
- minor instances of unused code being left in the codebase
| @@ -0,0 +1,181 @@ | |||
| import React, { useState, useRef, useEffect } from 'react'; | |||
There was a problem hiding this comment.
The MUI comment for the date picker applies to this to: it looks great, but for maintainability purposes I think just sticking with the built-in MUI components would be better
There was a problem hiding this comment.
Agreed -- also there are some weird things with the current date picker (e.g. it lists past and future dates within ten years so the menu is shifting based on the current selection which is kinda strange). It's not horrid and I think it looks great but we should probably use a prebuilt date picker, also makes accessibility and other standards easier to follow
There was a problem hiding this comment.
One other thing about time picker specifically -- I kinda expect to be able to use the time picker as one big text box (e.g. entering 0500 will output 5:00) but having to click on two separate boxes to enter text is counterintuitive, at least compared with how I expect to use components like this
| @@ -25,5 +25,21 @@ export const isTimeValid = (startDate: string, pickupTime: string) => { | |||
| return selectedTime.isSameOrAfter(now.add(bufferDays, 'day'), 'day'); | |||
| }; | |||
|
|
|||
| @@ -0,0 +1,363 @@ | |||
| import React, { useState, useRef, useEffect } from 'react'; | |||
There was a problem hiding this comment.
Consider 'lifting state' for the modal design:
instead of using a context, have the parent component (in this case, the CreateOrEditRideModal) define all formdata
i.e.
CreateOrEditRideModal {
currentFormData: formData
currentStep: 1 | 2 | 3 | 4
}
Then, rendering would look something like:
switch(currentStep) {
case: 1:
<Step1 formData={currentFormData} handleUpdateFormData={(newData)=> setFormData(newData)}></Step1>
}
Any child component-related props (i.e. whether selections are open/closed, what fields are unfilled, etc.) would be stored in the child components, and any functionality needed to update the parent would also belong entirely in the child component
- for instance, if a user modifies a value in one of the steps, the component would use the new value, create an updated version of the ride object, and then call
handleUpdateRide(updatedRide). The parent would update the ride value, which would handle any re-renders necessary. - States unique to a single step of the form, like whether a dropdown is open, should not be something that the parent component knows about
- Claude: "Object reconstruction is a non-issue at this scale. Creating a new object on each keystroke is negligible. React itself encourages immutable updates (new object rather than mutating). This is not a performance concern for a multi-step form."
|
[diff-counting] Significant lines: 7382. This diff might be too big! Developer leads are invited to review the code. |
mjaydenkim
left a comment
There was a problem hiding this comment.
This flow looks great -- good job on mobile styling and the logic of the overall flow, it makes a lot of sense! A few notes besides what I mention below:
- In general, better error reporting is needed. When an erroneous date/time/etc is entered, a lot of the time it will lead to a 400 server error which will not get shown to the user, which is not convenient. Please refer to #625, where I attempted to fix some of this -- I'm not sure how compatible it is with this PR but please implement that.
- When I try to create a ride I get an error, not sure if this is b/c of my recent merge or if it was pre-existing but we should talk it over and see if anyone else is getting the same --
Populate error: TypeError: Cannot read properties of undefined (reading 'getInternalProperties')
at PopulateItem (C:\Users\User\Documents\carriage-web\node_modules\.pnpm\dynamoose@4.1.5\node_modules\dynamoose\dist\Populate.js:15:28)
| } else { | ||
| return ( | ||
| notWeekend || | ||
| 'Please enter a valid date. (Note: CULifts does not operate during weekends or university-wide breaks.)' |
There was a problem hiding this comment.
More specific error messages would be good here
| if (isRecurringRide) { | ||
| // For now, block recurring rides as they're not fully implemented | ||
| alert( | ||
| 'Recurring rides are not yet supported. Please create a single ride instead.' |
There was a problem hiding this comment.
Probably doesn't matter since it won't be in production until later but having to go to the end of the workflow before this message appears is kinda annoying
| setShowLocationButton(false); | ||
| }} | ||
| > | ||
| Other (Custom Address) |
There was a problem hiding this comment.
i might be stupid but shouldn't we have a free-entry text form for this option?
There was a problem hiding this comment.
like to enter what the address actually is
There was a problem hiding this comment.
Oh yes I was meaning to ask this since it wasn't included in the design I left it for later. I think we should both let them pick in map and a textbox right
| @@ -0,0 +1,181 @@ | |||
| import React, { useState, useRef, useEffect } from 'react'; | |||
There was a problem hiding this comment.
One other thing about time picker specifically -- I kinda expect to be able to use the time picker as one big text box (e.g. entering 0500 will output 5:00) but having to click on two separate boxes to enter text is counterintuitive, at least compared with how I expect to use components like this







Summary
First step of the Request a Ride flow: multi-step modal (Date → Pickup → Dropoff → Summary) with desktop layout and inline editing on the summary step.
Features
-Date step: date picker (calendar popup), repeat options (No Repeat / Daily / Weekly / Biweekly / Custom), custom day selection
-Dropoff step: same as pickup (location + map + time input)
[x] Summary step: read-only summary with Edit per section; Edit opens inline editors for date/repeat, pickup (location + time), or dropoff (location + time) on the same screen
Test Plan



Manual
Both for desktop and mobile
Notes
-The calendar on top of request rides wizard and changes to the rides page are not included. I think current rides page changes require a seperate pr
-Existing time validation, and frontend check not enabling rides is the same
-In dropoff and pickup time I added an AM/PM selection button as in the current design it was underspecified how they would select that
-Right now electing the location "other" assumes we know the student's custom location. Do we want to add a form/pop-up for that too?
Breaking Changes
All changes are frontend-only and additive (new components and steps, optional className on Modal).