Skip to content
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

Add mini react-intl i18n implementation. #375

Merged
merged 23 commits into from
Jul 13, 2021

Conversation

philip-cline
Copy link
Contributor

Add a mini internationalization implementation to the default itinerary changing the mode names,
mode separator, the time format and the duration format. At the moment only english (en-US) and
french (fr-FR) translations are provided and more languages will be added in later pull requests.

Translated messages are defined in language-specific yml configuration files (e.g. en-US.yml) and all language yml files are loaded as dynamically assigned file names are not compatible with node.js.

For future development, the language setting is retrieved from the URL param 'locale' and may be used to change the displayed language.

Add a mini internationalization implementation to the default itinerary changing the mode names,
mode separator, the time format and the duration format. At the moment only english (en-US) and
french (fr-FR) translations are provided and more languages will be added in later pull requests.
lib/util/i18n.js Outdated
// (We cannot really load languages on demand using fetch... that would require tinkering with mastarm build,
// so here we build an index of translated messages.)
const translatedMessages = {
'en-US': require('../../i18n/en-US.yml'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that stands out to me here is that we may need to sub out these strings on a per-implementation basis. E.g., the names of otp modes in Portland OR may be different than in Atlanta (e.g. light rail vs streetcar for TRAM). Not really a requirement for right now, but any ideas for this at the moment?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this is needed, but think it'd be better to figure this out now rather than later. Also, I think it should be possible to load language files on-demand using fetch - the app should just wait until the base language file is loaded before the first main render. I think it'd make a lot of sense to have some kind of config file that can be used to override a default set of language files, however that process would likely be part of the trimet-mod-otp project (at least until we merge those two projects together).

Comment on lines 250 to 252
locale: getUrlParams().locale || 'en-US',
userLanguage: state.user.loggedInUser == null ? 'en-US' : state.user.loggedInUser.language,
use24HourFormat: state.user.loggedInUser == null ? false : state.user.loggedInUser.use24HourFormat
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These all deserve a little more thought on how they are derived. I think a good place a lot of this could live is somewhere in the ui object in the redux state. Even if a user has a default language or time format they might still want to change it to some other language.

Copy link
Contributor

@evansiroky evansiroky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See various comments, most of them are blocking. I think this is a good first demonstration of how things could work, but the overall structure of where the translations are loaded from and then stored in the redux state needs a more comprehensive design that will work everywhere else.

Restructuring language yml files, adding currency based on config settings, storing locale settings
in redux store
@landonreed landonreed changed the title feat(default-itinerary): Add mini react-intl i18n implementation. Add mini react-intl i18n implementation. Jun 2, 2021
Copy link
Member

@landonreed landonreed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good. See final comments about default lang/tolerance of different locale strings.

@landonreed landonreed removed their assignment Jun 14, 2021
Copy link
Contributor

@evansiroky evansiroky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address 1 comment, then this is good to go.

Copy link
Member

@landonreed landonreed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. Just a couple of non-blocking comments.


const mapStateToProps = (state, ownProps) => {
return {
currency: state.otp.config.localization?.currency || 'USD',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not be derived from GTFS or the relevant GBFS (or other) providers? I'm not sure if there's a strong justification as to why we need this property.

Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are displaying a cost line item for car-only, walk-only and own-bike-only itineraries, even if no transit itinerary is found and the cost shown is $0.00. We need an "ambient" currency that we don't know otherwise if no transit itinerary is shown.
The alternative is to not show the cost item if itinerary.fare is not set. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@landonreed says

Ah, yea, good point, Binh. I'm fine with the ambient currency. That's helpful info that might be useful to throw into a comment though.

Comments regarding this added in 9b7d311.

@landonreed landonreed removed their assignment Jun 29, 2021
@binh-dam-ibigroup binh-dam-ibigroup added the BLOCKED Blocked (waiting on another PR to be merged) label Jul 1, 2021
@binh-dam-ibigroup
Copy link
Collaborator

Adding the Blocked label, I would prefer we cut a release of OTP-RR before we start adding the internationalization features.

@binh-dam-ibigroup binh-dam-ibigroup removed the BLOCKED Blocked (waiting on another PR to be merged) label Jul 13, 2021
@binh-dam-ibigroup binh-dam-ibigroup merged commit f44c248 into opentripplanner:dev Jul 13, 2021
@github-actions
Copy link
Contributor

🎉 This PR is included in version 3.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants