-
Notifications
You must be signed in to change notification settings - Fork 59
Add Accessibility Labels to Legs and Itineraries #456
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
binh-dam-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.
The accessibility label texts need to support i18n.
Co-authored-by: binh-dam-ibigroup <56846598+binh-dam-ibigroup@users.noreply.github.com>
…p-react-redux into accessibility-routing
binh-dam-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.
One more minor comment for now.
binh-dam-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.
Please update the OTP-UI package versions for this PR.
|
Good catch Binh thanks! Totally slipped off my radar. Packages are updated and behavior seems to work! |
binh-dam-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.
One last change on the message id for the accessibility text.
binh-dam-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.
In example-config.yml, please move the accessibility localized text to be a child of the config section before merging.
philip-cline
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.
Looks great to me, just one small comment about file structure.
| /** | ||
| * Determine if an itinerary has accessibility scores | ||
| */ | ||
| export const itineraryHasAccessibilityScores = (itinerary) => { |
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.
For the i18n work, I was organising util functions into lib/util, and utility components like FormattedDuration into lib/components/util. Should these be moved to lib/util?
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.
Good point, if the function is used from multiple files then it should go to lib/util, if not, it should become private to this file.
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 was merging too many things this morning and accidentally forgot about this! Very sorry. The fix is in #487
|
🎉 This PR is included in version 3.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR depends on opentripplanner/otp-ui#289 to render the labels. Until these updates are released, imports (and therefore tests) will fail.
Adds labels to legs if they are provided by OTP, and generates a combined score for the itinerary
