-
Notifications
You must be signed in to change notification settings - Fork 58
Handle no pattern headsign #1499
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
amy-corson-ibigroup
left a comment
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.
Really nice improvement, thank you for all the different considerations on this! Just a few nitpicks/questions but overall looks good.
lib/util/viewer.js
Outdated
| // In case the headsign is blank, extract headsign from the pattern 'desc' attribute | ||
| // by attempting the logic below in order: | ||
| const attempts = [ | ||
| // (format: '49 to <Destination> (<destid>)[ from <Origin> (<originid)]'). |
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.
This is a nitpick, but can we remove the parenthesis around this comment? There's a lot of brackets and parenthesis happening in this line already and removing one set might make it easier to parse
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.
Yes, clarified in 52b7a44
| // If that regex didn't work, try the old regex | ||
| (d) => d?.match(/ to ([^(from)]+) \(.+\)/)?.[1], |
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.
Might be reading this regex wrong, but are there actual cases where this one would work and the above one would not? All the cases I'm trying, the regex on 83 does a better job
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 only copied over what was there before. I'm not sure which cases this one was supposed to cover.
alec-georgoff
left a comment
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.
Would it be worthwhile to also check for "toward [destination]" or "towards [destination]"? Not sure if any of those actually exist but it popped into my head.
@alec-georgoff I added the m in 0deb98a, let me know how that feels! |
Awesome, thanks! |
Description
This PR fixes the display of pattern headsigns for routes with a single pattern and no headsign. The headsign is the name of the last stop in the pattern, and the dropdown selector is not used. The PR also improves display of patterns with leading "To " text.
PR Checklist: