Skip to content
This repository has been archived by the owner on Oct 23, 2024. It is now read-only.

add localization support for DateRange #99

Merged
merged 11 commits into from
Jul 12, 2022
Merged

Conversation

aleksarad
Copy link
Contributor

@aleksarad aleksarad commented Mar 21, 2022

Story: https://vimeo.github.io/iris/sb/translate-daterange/?path=/docs/components-dates-daterange--controls

What this PR does

Adds localization support for DateRange component based on this lucidchart

How it does that

  • Adds a new optional prop locale to DateRange and Calendar (days of the week and months are handled inside the Calendar component)
  • The default value of locale is en, so any existing instances of DateRange in use will remain unchanged
  • Manually adds translations for strings used in the DateRange component
  • Strings and dates are localized based on the locale prop

@aleksarad aleksarad marked this pull request as ready for review March 21, 2022 17:49
@aleksarad aleksarad requested a review from a team as a code owner March 21, 2022 17:49
@seanmcintyre seanmcintyre added the 🆕 enhancement New feature or request label Mar 28, 2022
@seanmcintyre seanmcintyre added this to the 0.120.0 milestone Apr 25, 2022
@aleksarad aleksarad requested a review from a team as a code owner May 12, 2022 21:11
Copy link
Collaborator

@juliewongbandue juliewongbandue left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@tylerthegrove tylerthegrove left a comment

Choose a reason for hiding this comment

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

I spoke with @aleksarad about this on Slack, but I'll relay the conversation.

I see two problems with this approach:

  1. Tying the language to the locale
    This change restricts the locale for the date / time to only the 8 language options. This means that where 'en-GB' date format was supported based on a user's system setting, it will now be converted to 'en' format month-day-year.

See when I set my computer to en-GB the current Iris behavior
Screenshot 2022-05-24 at 14 12 01
vs the changes
Screenshot 2022-05-24 at 14 11 36

  1. Restricting the languages that can be used
    Although 7 may be the base Vimeo set of translated languages, this change doesn't allow adding any more languages without adding them into Iris. Long term I think we'd like to take an approach similar to Shopify where translations are passed into a provider. Using a translation prop would be more along those lines.

My proposal is to:

  1. remove the locale prop completely and go back to defaulting to the user's system settings for date / time.
  2. using a prop translation where an engineer would pass in key / value pairs for the component strings. eg(
    { apply: 'Apply', clear: 'Clear', startDate: 'Start Date}` )

We can have a translations file with keys for our base languages. Then it can be used like
import { translations } from @vimeo/iris/DateRange <DateRange translation={translations['en']} />

translations['en'] would be the default prop value.

@aleksarad
Copy link
Contributor Author

Made the changes mentioned above^

Something I want to draw attention to is the potential mismatch between browser locale and translated strings.

For example, if the component is translated to es, but the browser remains in en:
Screen Shot 2022-06-14 at 2 55 29 PM

Any date related values will remain unlocalized.

vs. translated to es and browser locale set to es, which looks consistent:

Screen Shot 2022-06-14 at 2 55 48 PM

I am unsure of how common the first scenario is (as in, do users ever change the language on Vimeo but not their browser), but just wanted to surface in case it is something we'd want to account for.

@juliewongbandue
Copy link
Collaborator

Made the changes mentioned above^

Something I want to draw attention to is the potential mismatch between browser locale and translated strings.

For example, if the component is translated to es, but the browser remains in en: Screen Shot 2022-06-14 at 2 55 29 PM

Any date related values will remain unlocalized.

vs. translated to es and browser locale set to es, which looks consistent:

Screen Shot 2022-06-14 at 2 55 48 PM

I am unsure of how common the first scenario is (as in, do users ever change the language on Vimeo but not their browser), but just wanted to surface in case it is something we'd want to account for.

Hey @aleksarad , this is a good catch and a great question. I'm not sure if users change the language on Vimeo but not in their browser... 🤔

Aside from that, this PR looks good to me.

@tylerthegrove
Copy link
Collaborator

Made the changes mentioned above^

Something I want to draw attention to is the potential mismatch between browser locale and translated strings.

For example, if the component is translated to es, but the browser remains in en: Screen Shot 2022-06-14 at 2 55 29 PM

Any date related values will remain unlocalized.

vs. translated to es and browser locale set to es, which looks consistent:

Screen Shot 2022-06-14 at 2 55 48 PM

I am unsure of how common the first scenario is (as in, do users ever change the language on Vimeo but not their browser), but just wanted to surface in case it is something we'd want to account for.

So let's include those Month and Day abbreviation strings in the translation. Similar to how Shopify handles it here. We can just add "months" and "daysAbbreviated" keys. We can always fallback to the browser if we want to.

@seanmcintyre seanmcintyre removed this from the 0.120.0 milestone Jun 29, 2022
@seanmcintyre seanmcintyre merged commit 84f5c15 into main Jul 12, 2022
@seanmcintyre seanmcintyre deleted the translate-daterange branch July 12, 2022 16:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🆕 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants