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

Added i18n translations to tile components #4936

Merged
merged 7 commits into from
Feb 8, 2025

Conversation

dwnoble
Copy link
Contributor

@dwnoble dwnoble commented Feb 6, 2025

  • Added intl. formatMessage calls to hardcoded strings in tile components
  • Re-generated client-side translation strings files
  • Added locale parameter to /api/place/displayname calls. (Still todo: Add locale param support to /api/place/name)
  • Updated place page locale string extractor (chart_config_extractor.py) to pull chart titles from new place page config
  • Fixed issue where extract_messages.sh was throwing an error when trying to read in .d.ts typescript files

@dwnoble dwnoble requested review from beets and gmechali February 6, 2025 23:49
Copy link
Contributor

@gmechali gmechali left a comment

Choose a reason for hiding this comment

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

Shoudl we add a webdriver test here in a different language to ensure the language replacement happens? I can imagine a couple ways how we could regress that unintentionally

Copy link
Contributor

@beets beets left a comment

Choose a reason for hiding this comment

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

nice! thanks for adding these strings

},
"chart-title-with-two-variables-and-location": {
"defaultMessage": "{variable1} Vs. {variable2} in {placeType} of {place}",
"description": "Chart title for a chart comparing two different variables, for places of a specific type within a place. For example, this could be Obesity Rate Vs. Income in States of USA, or Housing vs Poverty in Countries of Europe."
Copy link
Contributor

Choose a reason for hiding this comment

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

would be good to clarify the example title with quotes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"header-overview": {
"defaultMessage": "Overview",
"description": "Text for header or subheader of Overview charts on place pages."
},
"key_demographics": {
"defaultMessage": "Key Demographics",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be sentence case (along with some of these other titles below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

},
"viaGoogle": {
"defaultMessage": "via Google",
"description": "Indicator displayed when chart data comes from Google"
Copy link
Contributor

Choose a reason for hiding this comment

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

an example string where this is used would be helpful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dwnoble
Copy link
Contributor Author

dwnoble commented Feb 7, 2025

Shoudl we add a webdriver test here in a different language to ensure the language replacement happens? I can imagine a couple ways how we could regress that unintentionally

Added a basic test, and added a TODO to update some of the assertions once we have actual translations in place

@dwnoble dwnoble enabled auto-merge (squash) February 7, 2025 23:48
@dwnoble dwnoble merged commit cff600c into datacommonsorg:master Feb 8, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants