feat(itinerary): add ability to sort itineraries by fare #1411
Conversation
| @@ -453,10 +454,12 @@ export function addSortingCosts<T extends Itinerary>( | |||
| totalFareResult === null ? Number.MAX_VALUE : totalFareResult | |||
|
|
|||
| const rank = calculateItineraryCost(itinerary, config) | |||
| const transitFare = getFare(itinerary).transitFare || 0 | |||
There was a problem hiding this comment.
what happens if you just let this stay as undefined instead setting it to zero? does that mess up the sort?
There was a problem hiding this comment.
when the value is zero or undefined the sort does not change.
There was a problem hiding this comment.
I guess we can remove || 0 as well for cleaner code. I'll let second reviewer decide.
lib/util/itinerary.tsx
Outdated
| @@ -48,6 +48,7 @@ export interface ItineraryWithCO2Info extends Itinerary { | |||
| export interface ItineraryWithSortingCosts extends Itinerary { | |||
| rank: number | |||
| totalFare: number | |||
| transitFare?: number | |||
There was a problem hiding this comment.
I noticed this is an optional, which leads me to my next question. I think either you should remove the optional here or remove the || 0 below
There was a problem hiding this comment.
I'll remove the optional, as both values of zero and undefined do not change the sort order
There was a problem hiding this comment.
Maybe if we get an undefined fare we return .01 or something? So that the confirmed free trips are sorted above fares where we don't actually know.
There was a problem hiding this comment.
if a fare is undefined would that be a data issue? Should we try to handle data issues?
There was a problem hiding this comment.
If agencies don't provide us fares we display "no fare available" and i think that's fine to do, I just don't know how it fits into this PR
daniel-heppner-ibigroup
assigned
josh-willis-arcadis and unassigned
daniel-heppner-ibigroup
There was a problem hiding this comment.
Blocking because the logic to actually return the fare is commented out.
Wondering also if the user facing label "fare" should be changed to "transit fare"? I'm worried about people assuming we're incorporating rental bike price into the total fare for micromobility trips
|
There's also a distinction between a $0 fare and "no fare information". I don't think "no fare info" should be weighted the exact same as a free trip? Feels like if you sort by cheapest fare you should see free trips before you see n/a fares. Idk @daniel-heppner-ibigroup what are your thoughts on this |
josh-willis-arcadis
assigned
amy-corson-ibigroup and unassigned
amy-corson-ibigroup and
josh-willis-arcadis
Adds logic to sort itineraries by only transit fare. The difference between sorting by fare and cost is that itineraries with driving or biking have increased cost than just transit itineraries.
PR Checklist: