DT-5921 Add car ferry functionality #5121

VillePihlava · 2024-10-15T12:43:14Z

This should not be merged before the functionality from this pr can be used opentripplanner/OpenTripPlanner#5966,

Proposed Changes

Add car ferry functionality
- matka config
- waltti config
Direct car and cars on transit combination view
Add overflow icon
Remove leg text for itineraries for ferries in the matka config
Fix leg-before-line width for bikes from 6px to 8px
Fix color for the secondary icon in the alternative itinerary bar

…ar and disable leg text for ferries in the matka config.

vesameskanen

Good work. I suggested a fair amount of mainly cosmetic changes, which make the code easier to read and understand.

In general, try to avoid adding unncecessary validation checks 'just in case'. The code should fail fast and crash if variable values are not valid.

app/component/RouteNumberContainer.js

app/component/itinerary/CarLeg.js

vesameskanen · 2024-10-27T11:24:58Z

app/component/itinerary/CarLeg.js

+          <div className={cx('itinerary-leg-action', 'car')}>
+            <div className="itinerary-leg-action-content">


Merge these into one div

I did not change these for now. For example, there are similar divs in BicycleLeg, BikeParkLeg, CarParkLeg. Do you want all of these changed or none?

Ok I understand. I would love to see all code improving continuously. Please fix this unless it feels unreasonably big task.

These two divs have been merged everywhere I could find them.

vesameskanen · 2024-10-27T11:25:21Z

app/component/itinerary/CarLeg.js

+          <div className={cx('itinerary-leg-action', 'car')}>
+            <div className="itinerary-leg-action-content">


I did not change these for now. For example, there are similar divs in BicycleLeg, BikeParkLeg, CarParkLeg. Do you want all of these changed or none?

Same as above.

These two divs have been merged everywhere I could find them.

app/component/itinerary/ItineraryList.js

vesameskanen · 2024-10-28T05:19:31Z

app/component/itinerary/ItineraryListHeader.js

+              severityLevel={AlertSeverityLevelType.Info}
+            />
+          </div>
+          <div className="with-car-info-notification">


merge these 2 divs: with-car-info-notification and with-car-info-content

I removed with-bike-info-header and with-car-info-header in itinerary-list-header.scss, they were not used anymore. I merged with-bike-info-content and with-bike-info-notification as well as with-car-info-content and with-car-info-notification.

I noticed a new css class .with-car-info-notification-icon which seems to be unused.

I removed with-car-info-notification-icon and with-bike-info-notification-icon. The car class was accidentally created based off of the old bike icon class. Both seem unused and I removed them

vesameskanen · 2024-10-28T05:28:48Z

app/component/itinerary/ItineraryPage.js

+    settings.includeCarSuggestions &&
+    config.carBoardingModes?.FERRY !== undefined &&
+    carPublicPlan.carPublicItineraryCount > 0;
+
  const showAltBar =


carPublicPlan.carPublicItineraryCount > 0 is enough

FERRY checking is now removed. The idea behind checking for FERRY is that only in the configs that it is defined in, the UI will show ferries. Now there isn't a config field for removing carpublic itineraries in case they are found but not wanted. I did not remove settings.includeCarSuggestions.

Can there be car + public itineraries if includeCarSuggestions === false ? That would be surprising.

The logic gets a bit complicated because car itinerary is fetched for co2 comparison although user does not wish to get car itineraries.

Make sure that car + public won't get populated just because of co2 car routing. It should contain something only when user has selected car routing.

I had a double check here and forgot I checked this earlier. This is now only carPublicPlan.carPublicItineraryCount > 0

app/component/itinerary/ItineraryPage.js

vesameskanen · 2024-10-28T05:39:43Z

app/component/itinerary/ItineraryPage.js

+      (settings.includeCarSuggestions &&
+        (carPlan?.edges?.length ||
+          (config.carBoardingModes?.FERRY !== undefined &&
+            carPublicPlan?.edges?.length))));


(settings.includeCarSuggestions && carPlan?.edges?.length) ||
carPublicPlan?.edges?.length > 0

This assumes that the carpublicplan would be shown even without the setting. I think this is wrong. Do you agree? FERRY is now removed

Combination (settings.includeCarSuggestions && carPlan?.edges?.length) is used because carplan may be comouted for co2 comparison only.

This has now been changed to (settings.includeCarSuggestions && carPlan?.edges?.length) ||
carPublicPlan?.edges?.length > 0. I had a double check here and forgot I checked this earlier

…ry-changes

vesameskanen · 2024-10-30T07:06:58Z

There is a conflict.

vesameskanen

I found some new logic errors related to car+public itinerary list creation. Please read my review comments.

vesameskanen · 2024-10-30T07:47:56Z

app/util/planParamUtil.js

+      return (
+        transitModes.length > 0 &&
+        settings.includeCarSuggestions &&
+        config.carBoardingModes?.FERRY !== undefined


This seems wrong. I suggest:

settings.includeCarSuggestions && filterTransitModes(...).length > 0

I changed this to transitModes.length > 0 && settings.includeCarSuggestions. I didn't realize previously that transitModes is filtered earlier with filterTransitModes.

vesameskanen · 2024-10-30T07:51:33Z

app/component/itinerary/ItineraryPage.js

+      altStates[PLANTYPE.CAR][0].loading === LOADSTATE.DONE &&
+      altStates[PLANTYPE.CARTRANSIT][0].loading === LOADSTATE.DONE
+    ) {
+      const plan = mergeCarDirectAndTransitPlans(


I guess this is not OK. car + transit should be set only when there are carBoardingModes configured and settings.includeCarSuggestions is true.

This is very similar to how the mergeBikeTransitPlans works. Because the check for making the cartransit query is made in planParamUtil, this should be fine as is. Do you want me to change this and the way it works for bikes or should I leave both as is?

I now changed it to
altStates[PLANTYPE.CAR][0].loading === LOADSTATE.DONE &&
altStates[PLANTYPE.CARTRANSIT][0].loading === LOADSTATE.DONE &&
settings.includeCarSuggestions &&
config.carBoardingModes !== undefined

…n-icon CSS classes.

VillePihlava added 7 commits October 4, 2024 16:38

Change configs for cars on ferries.

ec65410

Add icons for cars on ferries.

b6904f1

Add translations for cars on ferries.

fc90478

Add functionality for cars on ferries.

6d83ac6

Remove unnecessary parts from script.

2953a88

Fix wrong color for the secondary icon in the alternative itinerary b…

a12ad0e

…ar and disable leg text for ferries in the matka config.

Fix bike leg-before-line width from 6px to 8px.

450fb49

VillePihlava requested a review from vesameskanen October 15, 2024 12:43

VillePihlava changed the title ~~DT-5921 Add car ferry functionality~~ [Do not merge] DT-5921 Add car ferry functionality Oct 15, 2024

vesameskanen requested changes Oct 28, 2024

View reviewed changes

VillePihlava added 2 commits October 29, 2024 14:01

Changes based on review comments.

319297d

Merge branch 'v3' of github.com:HSLdevcom/digitransit-ui into car-fer…

98e19d9

…ry-changes

vesameskanen requested changes Oct 30, 2024

View reviewed changes

VillePihlava changed the title ~~[Do not merge] DT-5921 Add car ferry functionality~~ DT-5921 Add car ferry functionality Oct 31, 2024

VillePihlava added 6 commits October 31, 2024 09:29

Merge itinerary-leg-action and itinerary-leg-action-content divs.

e9d2c35

Remove the ? from all uses of leg.to?.field and leg.from?.field.

c90f7d3

Remove with-car-info-notification-icon and with-bike-info-notificatio…

edf8305

…n-icon CSS classes.

Fix use of settings.includeCarSuggestions.

201b2af

Fix PLANTYPE.CARTRANSIT case in planParamUtil.

7a0a5a3

Add ? back to leg.to check to fix failing test.

d879234

VillePihlava marked this pull request as ready for review October 31, 2024 08:48

Add checks for mergeCarDirectAndTransitPlans.

21c4fbc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DT-5921 Add car ferry functionality #5121

DT-5921 Add car ferry functionality #5121

VillePihlava commented Oct 15, 2024 •

edited

Loading

vesameskanen left a comment

vesameskanen Oct 27, 2024

VillePihlava Oct 29, 2024

vesameskanen Oct 30, 2024

VillePihlava Oct 31, 2024

vesameskanen Oct 27, 2024

VillePihlava Oct 29, 2024

vesameskanen Oct 30, 2024

VillePihlava Oct 31, 2024

vesameskanen Oct 28, 2024

VillePihlava Oct 29, 2024

vesameskanen Oct 30, 2024

VillePihlava Oct 31, 2024

vesameskanen Oct 28, 2024

VillePihlava Oct 29, 2024

vesameskanen Oct 30, 2024 •

edited

Loading

VillePihlava Oct 31, 2024

vesameskanen Oct 28, 2024 •

edited

Loading

VillePihlava Oct 29, 2024

vesameskanen Oct 30, 2024

VillePihlava Oct 31, 2024

vesameskanen commented Oct 30, 2024

vesameskanen left a comment

vesameskanen Oct 30, 2024

VillePihlava Oct 31, 2024

vesameskanen Oct 30, 2024

VillePihlava Oct 31, 2024

VillePihlava Oct 31, 2024

		<div className={cx('itinerary-leg-action', 'car')}>
		<div className="itinerary-leg-action-content">

DT-5921 Add car ferry functionality #5121

Are you sure you want to change the base?

DT-5921 Add car ferry functionality #5121

Conversation

VillePihlava commented Oct 15, 2024 • edited Loading

Proposed Changes

vesameskanen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vesameskanen Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vesameskanen Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vesameskanen commented Oct 30, 2024

vesameskanen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VillePihlava commented Oct 15, 2024 •

edited

Loading

vesameskanen Oct 30, 2024 •

edited

Loading

vesameskanen Oct 28, 2024 •

edited

Loading