beamworks / react-csv-importer

Uploader + CSV parser + raw file preview + UI for custom user column mapping, all in one
https://www.npmjs.com/package/react-csv-importer
MIT License
231 stars 95 forks source link

Introduce Localization #57

Closed tstehr closed 2 years ago

tstehr commented 2 years ago

Hi folks πŸ‘‹

First of, big thanks for building this awesome library. It was really easy to integrate and works great πŸ‘

Since I work on a multilingual product, we need a localized UI to deploy this to users. I've taken a shot at implementing localization in this PR. Following the discussion in #28, my implementation is inspired by Material UI's localization.

High Level Overview

The PR introduces a LocaleContext that provides a complete locale object for all components. Components can use a useLocal hook to get their respective set of UI strings. A component locale can also contain functions that return strings, which allows templating like `Page ${page} of ${pageCount}`.

Users can use a bundled locale object, or provide their own which they can translate using the i18n tooling of their choice. In addition to the LocaleContext this PR also provides a german locale as a demonstration how future additional locales would look like.

Differences from Material UI

Implementing this I've slightly deviated from the Material UI approach. I believe the deviation is warranted by the differences between a library providing a set of components like Material and a library like this one which mainly provides a single component.

Material exposes their ThemeProvider to users and expects them to add it somewhere in the React component hierarchy:

<ThemeProvider theme={theme}>
  <App />
</ThemeProvider>;

This is necessary as <App /> will use several Material UI components in different places of the hierarchy intermixed with custom components, which all must have access to the same locale.

For react-csv-importer I chose to add locale as a prop to the Importer component since Importer is self contained. This keeps the interface easy to use.

import { Importer, ImporterField, deDE } from 'react-csv-importer';

<Importer
 ...
 locale={deDE}
>
 ...
</Importer>;

Wrapping Up

Again I'm very grateful for your work on react-csv-importer, it's an awesome library!

I'm open to iterate on this PR if you'd prefer a different approach to localization πŸ˜ƒ

unframework commented 2 years ago

Hey Tilman, wow, that's quite a PR, much appreciated! I am still getting familiar with the changes, and will need to revisit that old discussion as well. Thanks for submitting, will get back to you shortly - and glad that you are finding the library useful!

unframework commented 2 years ago

Hey @tstehr thanks again for the super detailed PR! Agreed, the context-based approach still makes sense and the prop-based locale injection in Importer component also seems most convenient.

One concern I have is that I would reorganize the i18n keys grouping by UX step or "screen" rather than individual internal React component. I am actually working to redo the internal React components right now so the structure will have to change a bit anyway. In general I don't want to get too stuck with the current internal naming - because if anyone makes a custom localization file they would have to rename stuff any time I refactor code.

The UX steps or screens will be roughly:

This may sound controversial, but what if we get rid of i18n key grouping altogether? Maybe could use a common prefix instead for any items in the same screen. Might be a pragmatic choice for a codebase that will change often.

unframework commented 2 years ago

Looks like my refactor has already introduced a lot of merge conflicts, really sorry about that! I can assist with the conflict resolution but it would be good to discuss the above points anyway.

tstehr commented 2 years ago

Hey @unframework thank you for your feedback!

Looks like my refactor has already introduced a lot of merge conflicts, really sorry about that! I can assist with the conflict resolution but it would be good to discuss the above points anyway.

No big deal, thank you for the heads up. I did the rebase and turns out the conflicts were mostly simple to fix :)

One concern I have is that I would reorganize the i18n keys grouping by UX step or "screen" rather than individual internal React component.

I can totally see that. To be honest, this is just an artifact of me following Material UI approach closely. I agree that we should avoid exposing all the internal components as essentially public API for localization.

This may sound controversial, but what if we get rid of i18n key grouping altogether?

I think that would work, given that there aren't too many strings to localize. I've done a quick look over all localization keys currently in use. There are only a couple of collisions and few names that would be too broad without the component name as context.

Both of these problems would be solved by prefixing with the step name or grouping according to step. I don't have a huge preference here. I slightly prefer prefixed over grouped, just because it makes changing a single string easier for users of the component.

unframework commented 2 years ago

As we discussed, I have a couple more naming convention changes I want to try, but I might as well merge your work first. I might tag you in a PR soon to ask for feedback on any further naming changes. Meanwhile, thanks again!