[WIP] [Locale] Intl C4R #741

menusal · 2023-07-13T21:06:28Z

Description

Shortcut: 145211

Use multi-language in projects

Type of change

(choose one and remove the others)

Feature

Acceptance

Please describe how to validate the feature or fix

go to X
test Y
assert Z

If feature deals with theme / UI or internal elements used also in CARTO 3, please also add a note on how to do acceptance on that part.

Basic checklist

Good PR name
Shortcut link
Changelog entry
Just one issue per PR
GitHub labels
Proper status & reviewers
Tests
Documentation

github-actions · 2023-07-13T21:11:36Z

Pull Request Test Coverage Report for Build 6236847753

48 of 49 (97.96%) changed or added relevant lines in 15 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 71.716%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/react-ui/src/widgets/TableWidgetUI/TableWidgetUI.js	2	3	66.67%

Totals
Change from base Build 6182739362:	0.2%
Covered Lines:	2301
Relevant Lines:	2972

💛 - Coveralls

github-actions · 2023-07-13T21:14:44Z

Visit the preview URL for this PR (updated for commit e4b04b6):

https://cartodb-fb-storybook-react-dev--pr741-feature-menusal-zassqukm.web.app

_{(expires Fri, 21 Jul 2023 07:01:04 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 517cc4d31d7e09cf277774e034094b67c301cd4c}

zbigg

looks good 👍 , only minor comments

packages/react-ui/src/widgets/TimeSeriesWidgetUI/TimeSeriesWidgetUI.js

packages/react-ui/src/hooks/useImperativeIntl.js

zbigg · 2023-07-19T13:35:30Z

packages/react-ui/src/widgets/legend/LegendWrapper.js

@@ -182,7 +187,14 @@ function Header({
        </Tooltip>
      )}
      {switchable && (
-        <Tooltip title={(visible ? 'Hide' : 'Show') + ' layer'}>


create two proper distinct translations for 'Hide layer' and 'Show layer'

(current approach assumes language without declension like polish: layer=warstwa, Show layer=Pokaż warstwę)

packages/react-ui/src/hooks/useImperativeIntl.js

packages/react-ui/src/localization/id-ID.js

packages/react-ui/src/widgets/RangeWidgetUI/RangeWidgetUI.js

VictorVelarde · 2023-07-25T08:49:16Z

Hi @aaranadev this is almost there.

If you can please have a general look / do some basic cleanup I think we'll be ready for an alpha. With the alpha, we will make a PR on cloud-native to check it works and we will appreciate a very small PR in carto-react-template to validate it works as expected

menusal · 2023-09-19T14:18:57Z

Hi @aaranadev this is almost there.

If you can please have a general look / do some basic cleanup I think we'll be ready for an alpha. With the alpha, we will make a PR on cloud-native to check it works and we will appreciate a very small PR in carto-react-template to validate it works as expected

@VictorVelarde @zbigg @aaranadev
I have resumed PR. Please check the latest commits.
Next step before merging the PR. Try using the Provider in the template and check that the language changes.

Update: After testing useImperative hook version into the C4R template without success, we need to discuss the following options:

Imperative API

Cannot be used with IntlProvider, use createIntl to create an intl object without using a provider.

Option A (currently implemented in the PR)

Expose a property to which the intl configuration is passed.

// Widget
<CategoryWidget
    id='revenueByStoreType'
    title='Category'
    dataSource={source.id}
    column='storetype'
    operationColumn='revenue'
    operation={AggregationTypes.SUM}
    intlConfig={INTL_CONFIG}
/>

Pros

The property is optional; the default configuration is used if not passed.
No need to create a provider for each component that needs translation.
Can be used in any component and leave others with the default translation.
Does not force the use of an Intl provider in applications that use a different translation system, such as react-i18next or a different version of react-intl than the one used in C4R.

Cons

A property is exposed in the widgets.

Use redux with the intl configuration to inject messages into the store and use them in any component that needs translation.

import { setItlConfig } from 'from '@carto/react-redux'

dispatch(setItlConfig(INTL_CONFIG))

Pros

No property is exposed. One dispatch for all translations.
No need to create a provider for each component that needs translation.
Does not force the use of an Intl provider in applications that use a different translation system, such as react-i18next or a different version of react-intl than the one used in C4R.

Cons

Dependency on redux.
All widgets are translated.
The intl configuration must be passed as a property to the UI of the widgets.

NOTE: Both options use the same object to pass the Intl configuration:

// External application using Carto for React
export const INTL_CONFIG = {
  locale: 'id',
  messages: {
    c4r: {
      widgets: {
        category: {
          apply: 'Apply',
          unlock: 'Unlock',
          lock: 'Lock',
          clear: 'Clear',
          noResults: 'No results',
          noResultsMessage:
            'Your search "{searchValue}" did not yield any matches.',
          // Rest of the messages
        }
      },
    },
  },
};

Option B: IntlProvider (Not tested)

IntlProvider

Create an IntlProvider provider with the intl configuration and use it in the component.

Pros
- No property is exposed in the widgets.
Cons
- All widgets are translated.
- Need to create an Intl provider in each component that needs translation.
- Users must use an Intl provider in their applications, even if they might want to use a different translation system, such as react-i18next or a different version of react-intl than the one used in C4R.

zbigg · 2023-10-03T07:38:01Z

packages/react-ui/src/hooks/useImperativeIntl.js

+  const messagesLocale = findMatchingMessagesLocale(locale);
+  const intMessages = intlConfig?.messages
+    ? flattenMessages(intlConfig?.messages)
+    : messages[messagesLocale];


i think all lines above should be in memo together with createIntl

mainly findMatchingMessagesLocale and flattenMessages are not trivial and everything depends only on intlConfig

zbigg · 2023-10-03T07:40:40Z

packages/react-widgets/src/widgets/TimeSeriesWidget.js

@@ -81,6 +81,7 @@ const EMPTY_ARRAY = [];
 * @param  {string[]=} [props.palette] - Optional palette
 * @param  {object} [props.wrapperProps] - Extra props to pass to [WrapperWidgetUI](https://storybook-react.carto.com/?path=/docs/widgets-wrapperwidgetui--default).
 * @param  {object} [props.noDataAlertProps] - Extra props to pass to [NoDataAlert]().
+ * @param  {object} [props.intlConfig] - Object with intl configuration. If not provided, default messages will be used.


please use {object=} to indicate that this is optional parameter

(not necessarily here, as there is proper .d.ts, but mainly for other widgets and places when you add optional param

I'm going to remove all intlConfig props, and we are to implement the Provider option.

menusal · 2023-10-18T07:57:38Z

Closed in favor to B option

Add react intl

4d740ac

menusal marked this pull request as draft July 13, 2023 21:06

menusal added 7 commits July 13, 2023 23:18

Add locales

774a99a

Add intl-localematcher

693f0d8

Add use imperative hook (wip)

39938cc

Replace harcoded messages usin intl hook

22025bf

Update feature modes id for test and storybook

acc7011

update test

b7aee68

remove unused props

e4b04b6

menusal requested a review from aaranadev July 14, 2023 07:07

wip override internal from external provider

1a28609

menusal requested review from VictorVelarde and zbigg July 14, 2023 11:49

zbigg reviewed Jul 19, 2023

View reviewed changes

VictorVelarde reviewed Jul 25, 2023

View reviewed changes

packages/react-ui/src/hooks/useImperativeIntl.js Outdated Show resolved Hide resolved

VictorVelarde reviewed Jul 25, 2023

View reviewed changes

packages/react-ui/src/localization/id-ID.js Outdated Show resolved Hide resolved

VictorVelarde reviewed Jul 25, 2023

View reviewed changes

packages/react-ui/src/widgets/RangeWidgetUI/RangeWidgetUI.js Outdated Show resolved Hide resolved

menusal added 5 commits September 18, 2023 12:52

Merge branch 'master' into feature/menusal/locales

7dcc75e

Update imperative hook

48b58ff

remove comments

057d1f6

fix typo

efea7b7

refactor tooltip actions labels

b9f480d

menusal marked this pull request as ready for review September 19, 2023 14:05

naming refactor for locale files

f989691

menusal requested review from VictorVelarde and zbigg September 19, 2023 14:19

menusal added 8 commits September 29, 2023 12:35

Merge branch 'master' into feature/menusal/locales

d712598

feat: update approach by intl-config prop

9080ac4

fix lint

7058381

Merge branch 'master' into feature/menusal/locales

0e441b3

remove non default languages

80b169c

remove consoles

f43ae83

remove unused keys

661af57

add intl to time series widget

34b7eff

menusal changed the title ~~[Locale] Intl C4R~~ [WIP] [Locale] Intl C4R Oct 2, 2023

menusal added the do not merge! label Oct 2, 2023

menusal requested a review from efernandezleon October 2, 2023 19:45

zbigg reviewed Oct 3, 2023

View reviewed changes

Add Intl props inside createInt useMemo

433cc9c

menusal marked this pull request as draft October 18, 2023 07:56

menusal closed this Oct 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] [Locale] Intl C4R #741

[WIP] [Locale] Intl C4R #741

menusal commented Jul 13, 2023

github-actions bot commented Jul 13, 2023 •

edited

Loading

github-actions bot commented Jul 13, 2023 •

edited

Loading

zbigg left a comment

zbigg Jul 19, 2023

menusal Sep 19, 2023

VictorVelarde commented Jul 25, 2023

menusal commented Sep 19, 2023 •

edited

Loading

zbigg Oct 3, 2023

menusal Oct 18, 2023

zbigg Oct 3, 2023

menusal Oct 18, 2023

menusal commented Oct 18, 2023

[WIP] [Locale] Intl C4R #741

[WIP] [Locale] Intl C4R #741

Conversation

menusal commented Jul 13, 2023

Description

Type of change

Acceptance

Basic checklist

github-actions bot commented Jul 13, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6236847753

💛 - Coveralls

github-actions bot commented Jul 13, 2023 • edited Loading

zbigg left a comment

Choose a reason for hiding this comment

zbigg Jul 19, 2023

Choose a reason for hiding this comment

menusal Sep 19, 2023

Choose a reason for hiding this comment

VictorVelarde commented Jul 25, 2023

menusal commented Sep 19, 2023 • edited Loading

Imperative API

Option A (currently implemented in the PR)

Option B: IntlProvider (Not tested)

zbigg Oct 3, 2023

Choose a reason for hiding this comment

menusal Oct 18, 2023

Choose a reason for hiding this comment

zbigg Oct 3, 2023

Choose a reason for hiding this comment

menusal Oct 18, 2023

Choose a reason for hiding this comment

menusal commented Oct 18, 2023

github-actions bot commented Jul 13, 2023 •

edited

Loading

github-actions bot commented Jul 13, 2023 •

edited

Loading

menusal commented Sep 19, 2023 •

edited

Loading