-
Notifications
You must be signed in to change notification settings - Fork 34
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
Itinerary Body: migrate to OTP2 trip props #640
Itinerary Body: migrate to OTP2 trip props #640
Conversation
packages/itinerary-body/src/TransitLegBody/view-trip-button.tsx
Outdated
Show resolved
Hide resolved
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 smell a breaking change so it will require a breaking change commit. To avoid a breaking change, fall back on fromIndex
and toIndex
if fromStopId
and toStopId
are not provided.
I think we have to make a breaking change. OTP1 is no longer supported, so it makes no sense to support it here. I will modify the commit message |
d926733
to
6bc7f1e
Compare
6bc7f1e
to
3599dc4
Compare
# Conflicts: # yarn.lock
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 you update the mocks for the "Individual Leg Fare Components" to have fromStopId
and toStopId
?
packages/itinerary-body/src/TransitLegBody/view-trip-button.tsx
Outdated
Show resolved
Hide resolved
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.
See #640 (review)
Updating the mocks would be a 2-release process... |
Why? The mocks are in the itinerary-body package. |
Wait sorry, I got myself confused. Mocks are updated |
I'm not sure what's going on... The new mocks aren't showing up in the GitHub diffs either, but they are showing up in my local git commit history |
Looks like everything is updated now, please try again @binh-dam-ibigroup |
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.
See https://github.com/opentripplanner/otp-ui/pull/640/files#r1310695054 that was the point of doing 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.
LGTM, sorry for the confusion!
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 the types could use some work, but this looks good... Thanks!
leg?.trip?.arrivalStoptime?.stopPosition | ||
} | ||
toIndex={leg.to?.stopIndex} | ||
toStopId={leg?.to?.stopId} |
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.
unneeded question mark
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.
What happens if there is no to
?
leg?.trip?.departureStoptime?.stopPosition | ||
} | ||
fromIndex={leg.from?.stopIndex} | ||
fromStopId={leg?.from?.stopId} |
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.
unneded question mark
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.
What happens if there is no from
The OTP1 trip props are no longer in use, and are fundamentally incompatible with the OTP2 props. This PR resolves this incongruity.