-
Notifications
You must be signed in to change notification settings - Fork 57
Include route.shortname in the transitive route label #1468
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
Conversation
lib/app.js
Outdated
@@ -167,7 +167,7 @@ const components = { | |||
* @returns A string with the custom label to display for the given leg, or null to render no label. | |||
*/ | |||
getTransitiveRouteLabel: (itineraryLeg) => { | |||
return itineraryLeg.routeShortName // null or undefined or empty string will tell transitive-js to not render a route label. | |||
return itineraryLeg.routeShortName || itineraryLeg.route.shortName // null or undefined or empty string will tell transitive-js to not render a route label. |
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.
Do we know route
will exist? Will it always have a shortname?
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 added a conditional to route
. If there's no short name, it'll return undefined
which in theory transitive should handle gracefully ( it doesn't but that's out of the scope of 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.
What if the route object is missing?
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.
Then it'll return undefined.
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'm nervous but I trust you!
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!!
Description:
Make sure all available short names are passed to transitive overlay.
A follow up OTP-UI PR is needed because the comment on this line (that a blank string will render no label) is not the case with the new transitive overlay bubbles.
PR Checklist: