-
Notifications
You must be signed in to change notification settings - Fork 54
April OTP-UI package updates #1373
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
Conversation
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.
Was there really no way to do the rounding work within react-intl?
itinerary.walkTime > 0 | ||
? ensureAtLeastOneMinute(itinerary.walkTime) | ||
: itinerary.walkTime |
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.
This is like taking a deep breath next to an exhaust pipe
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.
could we add a comment that explains that we need it to be exactly 0 in some cases, but any value between 1 - 59 is problematic!
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.
Should that condition also appear on lines 240 and 394, and 486?
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.
Added in c664f9b
We didn't look at react-intl. We can go back and explore options with roundingIncrement if you want but since these versions have already been released and they work I'd at least like to use them as a stop gap. |
react-intl does not (should not) handle calculations. |
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.
Should the non-zero duration check be used in more places?
itinerary.walkTime > 0 | ||
? ensureAtLeastOneMinute(itinerary.walkTime) | ||
: itinerary.walkTime | ||
} |
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.
Should this condition also appear on line 95?
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.
My thinking was it would be nearly impossible to have a leg/itinerary with an exact duration of 0 so it felt like an unnecessary check. However for itineraries that might not have any walk legs, we don't want to accidentally add a default walk time of 1min to an itinerary with absolutely no walking.
Although maybe when you're taking transit it's equally as unlikely to have itineraries with 0 walking, so we should just apply the check across the board to be safe?
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.
Leg with duration 0 is very common! That's how we identify transfer legs
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.
I think I'd prefer applying the check inside core-utils. Are we ok to block this PR?
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.
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.
Sorry I wasn't clear. The reason to block is to avoid having to check for explicitly 0 leg durations in multiple places
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.
I do not think we should have to check for explicitly 0 leg durations in multiple places. It makes sense to have this check here, because we could feasibly get an itinerary with a 0 walkDuration
and we would not want to replace that with a 1min. For example, you plan a trip that starts at one bus stop and ends at another bus stop.
Are there examples where we would need to show a 0min duration for a leg in the itinerary? A leg is going to take you from point A to point B and that's going to take time. The point of the util is to avoid implying that it's going to take 0 time.
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.
And just to be clear so I'm not contradicting myself, I know I said
so we should just apply the check across the board to be safe?
But thinking about it further, I don't think there's a reason to.
itinerary.walkTime > 0 | ||
? ensureAtLeastOneMinute(itinerary.walkTime) | ||
: itinerary.walkTime |
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.
Should that condition also appear on lines 240 and 394, and 486?
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.
I am good with the explanation ton only roundup non-zero walk leg durations.
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.
Since the 0 check is only happening in one place I'm happy to merge
Thanks everyone! |
Description:
Core utils: https://github.com/opentripplanner/otp-ui/releases/tag/%40opentripplanner%2Fcore-utils-v12.2.0
Geocoder: https://github.com/opentripplanner/otp-ui/releases/tag/%40opentripplanner%2Fgeocoder-v3.0.4
Itinerary body: https://github.com/opentripplanner/otp-ui/releases/tag/%40opentripplanner%2Fitinerary-body-v6.2.0
Also added
ensureAtLeastOneMinute
to itinerary durationsPR Checklist: