Fix route sort, color issues #123
Conversation
| // contrast color and use that if no text color is available. | ||
| const contrastColor = getContrastYIQ(backgroundColor) | ||
| const color = `#${defaultRouteTextColor || route.textColor || contrastColor}` | ||
| console.log(route, color, backgroundColor, contrastColor) |
There was a problem hiding this comment.
Please remove console.log
lib/util/itinerary.js
Outdated
| aComp = a.sortOrder | ||
| bComp = b.sortOrder | ||
| } else if (!isNaN(parseInt(a.shortName)) && !isNaN(parseInt(b.shortName))) { | ||
| // Otherwise, if both short names can be parsed as integers, use these | ||
| // numbers for sorting. |
There was a problem hiding this comment.
There is a chance that two routes could have the exact same number value for shortName. In that case, I think it would make sense to defer to sorting based off of shortName/longName.
There was a problem hiding this comment.
The viewer doesn't distinguish by agency, does it?
There was a problem hiding this comment.
@binh-dam-ibigroup, it used to, but it's been commented out:
otp-react-redux/lib/components/viewers/route-viewer.js
Lines 135 to 140 in 1210661
There was a problem hiding this comment.
@binh-dam-ibigroup, it does handle agencies:
otp-react-redux/lib/components/viewers/route-viewer.js
Lines 61 to 65 in 12d9034
Codecov Report
@@ Coverage Diff @@
## dev #123 +/- ##
==========================================
+ Coverage 10.62% 10.65% +0.03%
==========================================
Files 133 133
Lines 6081 6090 +9
Branches 1756 1758 +2
==========================================
+ Hits 646 649 +3
- Misses 4613 4618 +5
- Partials 822 823 +1
Continue to review full report at Codecov.
|
rename some functions, use consistent arguments and allow disabling integer route short name sorting
Make route comparator tolerate all kinds of data
|
🎉 This PR is included in version 0.13.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
evansiroky
added
the
ported-to-otp-ui
This PR fixes the route sort order issue in #122.
It also addresses some issues with route text color found with SEPTA. When viewing the route viewer (link), the route text color is not available to render the route label because it is not included in the OTP short response for route. See image with issue:
This fix uses a utility function (which should move to otp-ui soon) to identify the right contrasting color for a given bg color.