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

feat: Add i18n support to DatePicker component #990

Merged
merged 13 commits into from
Apr 20, 2021

Conversation

hannaliebl
Copy link
Contributor

@hannaliebl hannaliebl commented Mar 5, 2021

Summary

Add some localization props to the <DatePicker /> component.

This doesn't cover all the aria-labels used in these components, though. I don't exactly know how to handle this - there are a lot of them on all the subcomponents, and some are complex (they contain interpolated values, like here.) One approach is to trim this back a bit and just provide props for localizations of the months, days, and day abbreviations (so remove what I did with statusTranslations and selectedDateTranslation props.) This doesn't cover all the instructional aria content, though.

Related Issues or PRs

Closes #866

How To Test

I added a localization example to Storybook with some of the new props I added.

Screenshots (optional)

Screen Shot 2021-03-05 at 10 05 17 AM

@trussworks-infra-zz trussworks-infra-zz temporarily deployed to storybook March 5, 2021 15:25 Inactive
@trussworks-infra-zz trussworks-infra-zz temporarily deployed to storybook March 5, 2021 15:34 Inactive
Copy link
Contributor

@suzubara suzubara left a comment

Choose a reason for hiding this comment

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

@hannaliebl Thank you so much for taking this on, this is some great work! My main thought is that this is not the first component that needs localization and it won't be the last, so I'm wondering how we might want to establish a standard pattern for localization props that might work across components.

What do you think about defining an object that maps out all of the content needed for the DatePicker component, so that in order to change languages, users just need to pass in a single object of the same shape with translated content? I'm thinking of the strategy employed by i18next & other localization frameworks that use JSON or other data formats to store language strings.

I think it's also worth noting this brief discussion in the USWDS repo about handling localization: uswds/uswds#3445

@brandonlenz
Copy link
Contributor

brandonlenz commented Mar 15, 2021

My main thought is that this is not the first component that needs localization and it won't be the last, so I'm wondering how we might want to establish a standard pattern for localization props that might work across components.

@suzubara This is definitely a tough one.

Not related to this PR, but maybe worth thinking about ahead of time: I was thinking about the GovBanner component as well, where there is an icon inserted into the middle of at text block, which makes it hard to localize since grammatically, that icon could appear in different places in the sentence depending on the language. I imagine we could do some string replacing to handle this, where we expect $LOCK_ICON (or similar) to be present somewhere in the localized text. That similar to how the templating uswds uses does it: https://github.com/uswds/uswds/blob/develop/src/components/banner/banner.config.yml

If we choose to have full creative flexibility over how to implement this regardless of the uswds solution, I'm also curious if it'd be worth (or even feasible/possible) getting really react-y with it and using the ContextApi to define a localization structure with defaults, that could also be exposed for consumers.

@trussworks-infra-zz trussworks-infra-zz temporarily deployed to storybook April 2, 2021 15:16 Inactive
@trussworks-infra-zz trussworks-infra-zz temporarily deployed to storybook April 2, 2021 15:48 Inactive
@hannaliebl
Copy link
Contributor Author

Ok, this is ready to be looked at again, I added a localization prop to the DatePicker that is an object with all the strings that can be used as translations in the component.

@trussworks-infra-zz trussworks-infra-zz temporarily deployed to storybook April 9, 2021 00:02 Inactive
Copy link
Contributor

@suzubara suzubara left a comment

Choose a reason for hiding this comment

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

For some reason when viewing Storybook on this branch, I'm not seeing the Date picker stories at all. Maybe it just needs to be updated with main?

Minor nit-picky suggestions:

  • How about i18n for the prop name instead of localization? (i18n is widely accepted shorthand for internationalization)
  • What if we introduce a i18n.ts file in the component directory, that can store the shape of the prop as well as the default values in English?
    • Any component that requires passing in localized strings can use this pattern
    • It feels a little bit haphazard for the default values to be defined across the different components, and I think if all of the subcomponents were able to rely on the localization.stringName (or i18n.stringName) prop it would be just a little more organized. So for example, i18n would be a required prop on <DatePicker /> but it would default to the English object (imported from i18n.ts).

Fixing the storybook issue is the only blocker in my mind, the other comments are suggestions that can be made later on.

@trussworks-infra-zz trussworks-infra-zz temporarily deployed to storybook April 19, 2021 21:51 Inactive
@trussworks-infra-zz trussworks-infra-zz temporarily deployed to storybook April 19, 2021 22:19 Inactive
@trussworks-infra-zz trussworks-infra-zz temporarily deployed to storybook April 19, 2021 22:23 Inactive
Copy link
Contributor

@brandonlenz brandonlenz left a comment

Choose a reason for hiding this comment

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

Looks awesome!

@hannaliebl thanks for getting the bulk of this feature nicely in place!
@suzubara thanks for carrying it across the finish line!

I do have one question more for my own learning: This type of thing seems like a great candidate for using Context internally to share the localizations to the broader tree of components. In this example, since it's never passed as a prop solely to be passed another level deeper (the i18n prop is always used in addition to potentially being passed deeper), I don't have any qualms with doing it this way. When would Context be something to consider? For a more complex tree where props are being passed purely to be passed again to further sub-components?

@suzubara
Copy link
Contributor

Looks awesome!

@hannaliebl thanks for getting the bulk of this feature nicely in place!
@suzubara thanks for carrying it across the finish line!

I do have one question more for my own learning: This type of thing seems like a great candidate for using Context internally to share the localizations to the broader tree of components. In this example, since it's never passed as a prop solely to be passed another level deeper (the i18n prop is always used in addition to potentially being passed deeper), I don't have any qualms with doing it this way. When would Context be something to consider? For a more complex tree where props are being passed purely to be passed again to further sub-components?

ooh, to be honest using Context for a tree of subcomponents didn't occur to me and I definitely think that would be a good way to build on this!

@suzubara suzubara merged commit 2000b9c into main Apr 20, 2021
@suzubara suzubara deleted the hannaliebl-i18n-datepicker-866 branch April 20, 2021 14:59
brandonlenz added a commit that referenced this pull request May 12, 2021
* Add localization props to DatePicker and Calendar, add WIP example

* Pass down localization props to Day and MonthPicker components

* Add some tests for localization props

* Pass selected date translation to Calendar

* Add changes I forgot to save

* Replace individual props with localization object

* Finish adding localization props, update tests

* Move localization interface and default to i18n file

* Rename default to EN_US, use as default value of localization prop

* Rename prop to i18n

Co-authored-by: Brandon Lenz <brandonalenz@gmail.com>
Co-authored-by: Suzanne Rozier <suz@truss.works>
@haworku haworku added the i18n Relates to internationalization or localization standards label Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n Relates to internationalization or localization standards
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feat] Add i18n support to DatePicker component
5 participants