-
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
Fare Table comaptibility with OTP2 FareProduct API #612
Fare Table comaptibility with OTP2 FareProduct API #612
Conversation
BREAKING CHANGE: Removes old getTransitFare() function
Fare data is now stored on the legs themselves, so it’s not necessary to pass it around on its own
BREAKING CHANGE: removes support for REST data in FareTable
Because of some internal dependency complications, we had to undo this change. We instead will have to merge all this first and then create a follow up PR that removes depending on alpha versions |
The types PR should contain a breaking change commit. |
Ok. Still needs a breaking change to the types package. |
BREAKING CHANGE: Removes support for older OTP1/REST legs
Great point, I cherry picked my breaking change commit over to this PR now for types. |
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 comment for types and core-utils packages (more comments to come)
packages/core-utils/src/__tests__/__snapshots__/itinerary.js.snap
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.
Needs a few more breaking change commits and some cleanup. Also, fares are no longer shown for the regular transit legs and for http://localhost:5555/?path=/story/tripdetails--fare-components-itinerary&globals=locale:en-US and http://localhost:5555/?path=/story/itinerarybody-otp-react-redux--individual-leg-fare-components&globals=locale:en-US, so either the fares should be shown, or the story title/contents should be updated.
@@ -35,7 +35,6 @@ const ItineraryBody = ({ | |||
showElevationProfile, | |||
showLegIcon, | |||
showMapButtonColumn = true, | |||
showRouteFares, |
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 should be a breaking change commit to the itinerary-body package.
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.
Also remove this prop from packages/itinerary-body/src/types.ts
.
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.
removed and breaking change added in 48bdcc4.
@@ -150,7 +140,8 @@ class TransitLegBody extends Component<Props, State> { | |||
timeZone, | |||
TransitLegSubheader, | |||
TransitLegSummary, | |||
transitOperator | |||
transitOperator, | |||
defaultFareSelector |
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.
Sort props.
</S.TransitLegExpandedBody> | ||
</AnimateHeight> | ||
|
||
{legCost && ( |
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 a request for moving the fare out of the expanding section?
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.
No, that was a mistake.
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 did some work to fix the fares displaying in the itinerary body. Lots of prop drilling, we should return to clean this up.
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.
Also includes breaking change for itinerary-body.
packages/trip-details/src/types.ts
Outdated
export interface TransitFareProps { | ||
fareKey: string; | ||
headerKey: string; | ||
fareMediumId: string |
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.
Sort props. There should be a breaking change commit for the trips-detail package for the props removed.
…or prop BREAKING CHANGE: Removes unused showRouteFares
BREAKING CHANGE: Removes types associated with old REST API
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.
Much cleaner now. See the few more items. Thanks for the changes!
packages/itinerary-body/src/stories/OtpRrItineraryBody.story.tsx
Outdated
Show resolved
Hide resolved
638a8e8
into
opentripplanner:master
No description provided.