goooseman / storybook-addon-i18n

Storybook I18n Addon can be used to change locale of the component inside the preview in storybook
34 stars 3 forks source link

Discussion in locale change detection #6

Closed taehwanno closed 5 years ago

taehwanno commented 5 years ago

Overview

About detecting of language change in react component, storybook-addon-i18n depends on providerLocaleKey in a storybook parameter. when a developer use react-intl, i18n addon can easily work. but about react-i18next, it doesn't support props like locale except i18n. So, additional boilerplate codes are needed.

I think it's better to choice adding new API (onChanged detection handler?) or update the documentation in README's complex usage.

Possible Directions

You can see the codes about each direction

1. Add new API (onChanged, the name can be changed)

You can see implementation in taehwanno/storybook-addin-i18n

// .storybook/config.js

import { I18nextProvider } from 'react-i18next' ;
import { addParameters } from '@storybook/react';
import i18n from '../src/i18n';

addParameters({
  i18n: {
    provider: I18nextProvider,
    providerProps: {
      i18n,
    },
    providerLocaleKey: 'locale',
    supportedLocales: ['ko', 'en'],
    onChanged: (locale) => i18n.changeLanguage(locale), // <-- new API
  },
});

2. Update the documentation in README's complex usage

// I18nProviderWrapper.jsx

import React, { useEffect } from 'react';
import { I18nextProvider } from 'react-i18next';

function I18nProviderWrapper({ children, i18n, locale }) {
  useEffect(() => {
    i18n.changeLanguage(locale); // <-- Change language with React effect hook
  }, [i18n, locale]);
  return <I18nextProvider i18n={i18n}>{children}</I18nextProvider>;
}

export default I18nProviderWrapper;
// .storybook/config.js

import { addParameters } from '@storybook/react';

import i18n from '../src/i18n';
import I18nProviderWrapper from './I18nProviderWrapper';

addParameters({
  i18n: {
    provider: I18nProviderWrapper, // <-- wrapped i18n provider
    providerProps: {
      i18n,
    },
    providerLocaleKey: 'locale',
    supportedLocales: ['ko', 'en'],
  },
});

getDirection is called every re-render, we can use this as a hook for language detection. but I think this isn't a proper direction given the original purpose of getDirection API.

// .storybook/config.js

import { I18nextProvider } from 'react-i18next' ;
import { addParameters } from '@storybook/react';
import i18n from '../src/i18n';

addParameters({
  i18n: {
    provider: I18nextProvider,
    providerProps: {
      i18n,
    },
    providerLocaleKey: 'locale',
    supportedLocales: ['ko', 'en'],
    getDirection: locale => {
      i18n.changeLanguage(locale); // <-- Change language based on React render phase
      return 'ltr';
    }
  },
});

Preferred Direction

I'm fine either way, whether in the direction of adding a new API or updating README documentation for a provider wrapper. Please give me an opinion :)

alexeychikk commented 5 years ago

@taehwanno Thanks for your feedback! As for adding onChanged event, I think Provider wrapper is a better way to handle this. This is not workaround, this is how it's meant to be. We can't adjust our API to every possible i18n provider so we expose provider component parameter as a part of our public API so that devs can adjust it to their own needs.
Anyway, adding your example to the README is a good idea.

As for direction, there is an issue with detecting the right direction #4 . I think after resolving it you won't need your own solution to it. I can make a PR today to fix it.
Also, I don't quite understand why you need to call i18n.changeLanguage(locale); inside getDirection? I think you should handle this inside your own provider (wrapper) that you pass as provider.

Your PR is welcome. Thanks!

alexeychikk commented 5 years ago

@taehwanno Also, if you are interested in a lightweight localization solution for your React app then you should take a look at https://github.com/trucknet-io/react-targem

goooseman commented 5 years ago

4 is resolved and a new version has been published

taehwanno commented 5 years ago

As for adding onChanged event, I think Provider wrapper is a better way to handle this. This is not >workaround, this is how it's meant to be. We can't adjust our API to every possible i18n provider so > we expose provider component parameter as a part of our public API so that devs can adjust it to > their own needs. Anyway, adding your example to the README is a good idea.

Thanks for the details. I understand API's purpose. I will add the example in README.

As for direction, there is an issue with detecting the right direction #4 . I think after resolving it you won't need your own solution to it. I can make a PR today to fix it.

šŸ‘

Also, I don't quite understand why you need to call i18n.changeLanguage(locale); inside getDirection? I think you should handle this inside your own provider (wrapper) that you pass as provider.

The direction that calls i18n.changeLanguage(locale) inside getDirection is an example of the possible workaround directions. I think you don't have to care about it :)