Skip to content

Conversation

nimzco
Copy link

@nimzco nimzco commented Feb 11, 2019

WHY are these changes introduced?

Resolves https://github.com/Shopify/intl-domestic-country-fit/issues/340

Currently, months and week names are hard coded in English, therefore, it breaks in other languages. See:
image

WHAT is this pull request doing?

  • Extract strings in en.json and use intl.translate to load them based on the month or weekday

How to 🎩

  • Replace the new translations in en.json and see them rendered on the storybook.

🎩 checklist

@BPScott BPScott temporarily deployed to polaris-react-pr-1005 February 11, 2019 20:48 Inactive
@nimzco
Copy link
Author

nimzco commented Feb 11, 2019

Oh, I didn't realize @joshuajay was taking a stab at this here: #874 🙈

I guess I can close this then @joshuajay?

@joshuajay
Copy link
Contributor

joshuajay commented Feb 11, 2019

They both handle the localisation in a different way, not sure which is better.

Mine will take date formats into account when displaying the month/year together and does not need to be maintained by Shopify's translation efforts since it leverages an already implemented international standard for localising date formatting.

This PR gives us more control over the translations, so there might be value in this over the PR I opened.

Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

🎩 works great (no re-render flash) and code looks 🏅Just update UNRELEASED.md and ship 🚢 and then add the translations to web/app/locales

"november": "November",
"december": "December"
},
"daysAbbr": {
Copy link
Member

Choose a reason for hiding this comment

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

For clarity and for folks who don't speak English as a first language, I think we should spell this out.

Suggested change
"daysAbbr": {
"daysAbbreviated": {

@nimzco
Copy link
Author

nimzco commented Feb 13, 2019

Awesome, thank you Chloe! 🙏

@nimzco nimzco merged commit e3d42d4 into master Feb 13, 2019
@nimzco nimzco deleted the extract_datepicker_strings branch February 13, 2019 21:36
@alex-page alex-page temporarily deployed to production February 20, 2019 15:24 Inactive
@alex-page alex-page temporarily deployed to production February 20, 2019 15:31 Inactive
@alex-page alex-page temporarily deployed to production February 20, 2019 15:40 Inactive
@alex-page alex-page temporarily deployed to production February 20, 2019 15:45 Inactive
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.

5 participants