Skip to content

[i18n] Remove i18n token skipping from EUI pluralization defString functions #129144

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.

Sign up for GitHub

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

Merged
cee-chen merged 3 commits into from
Apr 1, 2022
Merged

[i18n] Remove i18n token skipping from EUI pluralization defString functions #129144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c6c603
Update tests to compare EUI pluralization function output against Kib…
cee-chen Mar 31, 2022
e081a95
Fix mismatched EUI/Kibana default message
cee-chen Mar 31, 2022
3084457
Merge branch 'main' into 110132/i18n-eui-mapping
constancecchen Apr 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 36 additions & 29 deletions src/core/public/i18n/i18n_eui_mapping.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { getEuiContextMapping } from './i18n_eui_mapping';
const VALUES_REGEXP = /\{\w+\}/;

describe('@elastic/eui i18n tokens', () => {
const i18nTranslateActual = jest.requireActual('@kbn/i18n').i18n.translate;
Copy link
Copy Markdown
Contributor Author

@cee-chen cee-chen Mar 31, 2022

Choose a reason for hiding this comment

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

I wanted this to actually compare final string outputs to each other as I didn't think it was as useful / readable to try and compare the EUI output to some contrived pre-pluralization Kibana version.

const i18nTranslateMock = jest
.fn()
.mockImplementation((id, { defaultMessage }) => defaultMessage);
Expand Down Expand Up @@ -74,34 +75,9 @@ describe('@elastic/eui i18n tokens', () => {
});

test('defaultMessage is in sync with defString', () => {
// Certain complex tokens (e.g. ones that have a function as a defaultMessage)
// need custom i18n handling, and can't be checked for basic defString equality
const tokensToSkip = [
'euiColumnSorting.buttonActive',
'euiSelectable.searchResults',
'euiPrettyDuration.lastDurationSeconds',
'euiPrettyDuration.nextDurationSeconds',
'euiPrettyDuration.lastDurationMinutes',
'euiPrettyDuration.nextDurationMinutes',
'euiPrettyDuration.lastDurationHours',
'euiPrettyDuration.nextDurationHours',
'euiPrettyDuration.lastDurationDays',
'euiPrettyDuration.nexttDurationDays',
'euiPrettyDuration.lastDurationWeeks',
'euiPrettyDuration.nextDurationWeeks',
'euiPrettyDuration.lastDurationMonths',
'euiPrettyDuration.nextDurationMonths',
'euiPrettyDuration.lastDurationYears',
'euiPrettyDuration.nextDurationYears',
'euiPrettyInterval.seconds',
'euiPrettyInterval.minutes',
'euiPrettyInterval.hours',
'euiPrettyInterval.days',
'euiPrettyInterval.weeks',
'euiPrettyInterval.months',
'euiPrettyInterval.years',
];
if (tokensToSkip.includes(token)) return;
const isDefFunction = defString.includes('}) =>');
const isPluralizationDefFunction =
defString.includes(' === 1 ?') || defString.includes(' > 1 ?');

// Clean up typical errors from the `@elastic/eui` extraction token tool
const normalizedDefString = defString
Expand All @@ -114,7 +90,38 @@ describe('@elastic/eui i18n tokens', () => {
.replace(/\s{2,}/g, ' ')
.trim();

expect(i18nTranslateCall[1].defaultMessage).toBe(normalizedDefString);
if (!isDefFunction) {
expect(i18nTranslateCall[1].defaultMessage).toBe(normalizedDefString);
} else {
// Certain EUI defStrings are actually functions (that currently primarily handle
// pluralization). To check EUI's pluralization against Kibana's pluralization, we
// need to eval the defString and then actually i18n.translate & compare the 2 outputs
const defFunction = eval(defString); // eslint-disable-line no-eval
const defFunctionArg = normalizedDefString.split('({ ')[1].split('})')[0]; // TODO: All EUI pluralization fns currently only pass 1 arg. If this changes in the future and 2 args are passed, we'll need to do some extra splitting by ','
Copy link
Copy Markdown
Contributor Author

@cee-chen cee-chen Mar 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just want to highlight this TODO - I'm not sure when we would ever pass multiple args into an EUI pluralization fn, but if we ever do in the future this logic will need to be reworked because all I'm essentially doing here is isolating the text in '({ someVar })' to get a single variable name


if (isPluralizationDefFunction) {
const singularValue = { [defFunctionArg]: 1 };
expect(
i18nTranslateActual(token, {
defaultMessage: i18nTranslateCall[1].defaultMessage,
values: singularValue,
})
).toEqual(defFunction(singularValue));

const pluralValue = { [defFunctionArg]: 2 };
expect(
i18nTranslateActual(token, {
defaultMessage: i18nTranslateCall[1].defaultMessage,
values: pluralValue,
})
).toEqual(defFunction(pluralValue));
} else {
throw new Error(
`We currently only have logic written for EUI pluralization def functions.
This is a new type of def function that will need custom logic written for it.`
);
Comment on lines +119 to +122
Copy link
Copy Markdown
Contributor Author

@cee-chen cee-chen Mar 31, 2022

Choose a reason for hiding this comment

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

I can't imagine when EUI will ever add function interpolation that isn't for pluralization (see #110132 (comment) to confirm), but if we ever do, I thought it would be prudent to leave this thrown error in for us so that whoever is upgrading the EUI version catches it

}
}
});

test('values should match', () => {
Expand Down
2 changes: 1 addition & 1 deletion src/core/public/i18n/i18n_eui_mapping.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1268,7 +1268,7 @@ export const getEuiContextMapping = (): EuiTokensObject => {
),
'euiSelectable.searchResults': ({ resultsLength }: EuiValues) =>
i18n.translate('core.euiSelectable.searchResults', {
defaultMessage: '{resultsLength, plural, one {# result} other {# results}}',
defaultMessage: '{resultsLength, plural, one {# result} other {# results}} available',
Copy link
Copy Markdown
Contributor Author

@cee-chen cee-chen Mar 31, 2022

Choose a reason for hiding this comment

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

The new tests caught this default message mistake from a few upgrades ago. Hooray! :)

values: { resultsLength },
}),
'euiSelectable.placeholderName': i18n.translate('core.euiSelectable.placeholderName', {
Expand Down