-
Notifications
You must be signed in to change notification settings - Fork 132
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
DT-6182 New designs for scooters #5073
Conversation
app/util/planParamUtil.js
Outdated
moment(parseDateTime(arriveBy, time).earliestDeparture).diff( | ||
moment(), | ||
'minutes', | ||
) < config.vehicleRental.maxMinutesToRentalJourneyStart |
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.
Does this work correctly when arriveBy== true? Code seems to compare moment(undefined) with current time. Maybe arriveBy case needs different logic. If time is tomorrow, scooters surely should not be used.
Also, use of parseDateTime here seems totally unnecessary as that only converts unix time to a string and then you convert it back to moment object. Please use Date.now() to get epoch ms and compare it to time prop which is epoch seconds using simple math computation. We are trying to get rid of moment.
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.
ArriveBy was accidentally overlooked. Also forgot that we are getting rid of moment. The arriveBy limit is a little hard to define as a journey could take a long time but a scooter journey might be relevant in the beginning, although there is no guarantee that the suggestion would be in the beginning of the journey. I added another configuration for the arriveBy time constraint.
Some suggestions:
|
Proposed Changes
Pull Request Check List
Review