April OTP-UI package updates #1373
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import { | |
| IntlShape | ||
| } from 'react-intl' | ||
| import { Leaf } from '@styled-icons/fa-solid/Leaf' | ||
| import coreUtils from '@opentripplanner/core-utils' | ||
| import React from 'react' | ||
| import styled, { keyframes } from 'styled-components' | ||
|
|
||
|
|
@@ -41,6 +42,8 @@ import DepartureTimesList, { | |
| import MetroItineraryRoutes from './metro-itinerary-routes' | ||
| import RouteBlock from './route-block' | ||
|
|
||
| const { ensureAtLeastOneMinute } = coreUtils.time | ||
|
|
||
| // Styled components | ||
| const ItineraryWrapper = styled.div.attrs((props) => { | ||
| return { 'aria-label': props['aria-label'] } | ||
|
|
@@ -232,7 +235,11 @@ class MetroItinerary extends NarrativeItinerary { | |
| aria-hidden | ||
| footer={ | ||
| showLegDurations && | ||
| mainLeg?.duration && ( | ||
| <FormattedDuration | ||
| duration={ensureAtLeastOneMinute(mainLeg.duration)} | ||
| /> | ||
| ) | ||
| } | ||
| hideLongName | ||
| leg={mainLeg} | ||
|
|
@@ -384,7 +391,7 @@ class MetroItinerary extends NarrativeItinerary { | |
| > | ||
| <PrimaryInfo> | ||
| <FormattedDuration | ||
| duration={ensureAtLeastOneMinute(itinerary.duration)} | ||
| includeSeconds={false} | ||
| /> | ||
| </PrimaryInfo> | ||
|
|
@@ -409,7 +416,13 @@ class MetroItinerary extends NarrativeItinerary { | |
| values={{ | ||
| time: ( | ||
| <FormattedDuration | ||
| duration={ | ||
| /* If the walk time is truly zero, show 0. But if the walk time is just less | ||
| than a minute, round up to the nearest minute to avoid showing no walk time. */ | ||
| itinerary.walkTime > 0 | ||
| ? ensureAtLeastOneMinute(itinerary.walkTime) | ||
| : itinerary.walkTime | ||
|
Comment on lines
+422
to
+424
There was a problem hiding this comment. This is like taking a deep breath next to an exhaust pipe There was a problem hiding this comment. 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. Should that condition also appear on lines 240 and 394, and 486? There was a problem hiding this comment. Added in c664f9b |
||
| } | ||
| includeSeconds={false} | ||
| /> | ||
| ) | ||
|
|
@@ -470,7 +483,7 @@ class MetroItinerary extends NarrativeItinerary { | |
| <ItineraryGridSmall className="other-itin"> | ||
| <PrimaryInfo as="span"> | ||
| <FormattedDuration | ||
| duration={ensureAtLeastOneMinute(itinerary.duration)} | ||
| includeSeconds={false} | ||
| /> | ||
| </PrimaryInfo> | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
First of all, it's not how we identify transfer legs, we identify them by checking if
leg.to.stopId === leg.from.stopIdSecond of all, we don't show the leg duration on transfer legs:
So I don't understand why this would be a reason to block the 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.
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
walkDurationand 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
But thinking about it further, I don't think there's a reason to.