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

[EUI i18n token mapping] Long-term approach for complex defStrings #110132

Closed
cee-chen opened this issue Aug 25, 2021 · 4 comments · Fixed by #129144
Closed

[EUI i18n token mapping] Long-term approach for complex defStrings #110132

cee-chen opened this issue Aug 25, 2021 · 4 comments · Fixed by #129144
Assignees
Labels
Feature:Unit Testing Project:i18n Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc technical debt Improvement of the software architecture and operational architecture

Comments

@cee-chen
Copy link
Contributor

cee-chen commented Aug 25, 2021

Original PR discussion: #109926 (comment)

Summary: EUI has previously not pluralized / passed defaultMessages that translate to complex defStrings. The above PR is the first case of that instance, but it's possible we may pluralize more messages going forward. This leads to defStrings that are essentially stringified functions, and don not pass the defaultMessage is in sync with defString tests.

For the above PR, I went with skipping a single test for now, but I'm opening this issue so y'all can potentially track a better long-term solution for either testing or skipping complex defString cases.

@cee-chen cee-chen added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Aug 25, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@afharo
Copy link
Member

afharo commented Aug 26, 2021

Since the only use-case we've found so far is about Pluralization, I'd vote for a regexp to identify these patterns according to our guidelines and attempt to match the strings with that.

Ideally, we'd be able to call both methods with 0, 1 & 2, and match the results?

What do you think?

@cee-chen
Copy link
Contributor Author

cee-chen commented Aug 26, 2021

I agree pluralization is likely the most common case. @chandlerprall, can you think of any other complex i18n scenarios that might come up in the future?

I did a quick grep of EUI's i18ntokens_changelog just to see what other instances we had used functions in defStrings for in the past, and it looks like these are old ones that we eventually removed/refactored:

"({\n  status\n}: {\n  status: EuiTourStepStatus;\n}) => {\n  return `Step ${number} ${status}`;\n};"
// This one is weird and I'm not sure correct, it looks like it should have been a straight pass-through

"({\n  step,\n  title,\n  disabled,\n  isComplete\n}: Pick<EuiStepHorizontalProps, 'step' | 'title' | 'disabled' | 'isComplete'>) => {\n  let titleAppendix = '';\n\n  if (disabled) {\n    titleAppendix = ' is disabled';\n  } else if (isComplete) {\n    titleAppendix = ' is complete';\n  }\n\n  return `Step ${step}: ${title}${titleAppendix}`;\n};"
// This looks like it should have been 2 different strings instead of a conditional

"({\n  status\n}: {\n  status?: EuiStepStatus;\n}) => {\n  if (status === 'incomplete') return 'Incomplete Step';\n  return 'Step';\n};"
// Similar to above, just use 2 different i18n tokens instead of a conditional

"({\n  count,\n  hasActiveFilters\n}) => `${count} ${hasActiveFilters ? 'active' : 'available'} filters`;"
// Similar to above, 2 separate tokens/strings instead of a conditional

If nothing else, identifying functions might help us avoid unnecessary conditional i18n in the future :)

@chandlerprall
Copy link
Contributor

chandlerprall commented Aug 26, 2021

can you think of any other complex i18n scenarios that might come up in the future?

Nothing immediately comes to mind; good to start with the plural case and we can expand that if needed in the future 👍

If nothing else, identifying functions might help us avoid unnecessary conditional i18n in the future :)

🥇

@afharo afharo added EnableJiraSync technical debt Improvement of the software architecture and operational architecture labels Mar 30, 2022
@cee-chen cee-chen self-assigned this Mar 31, 2022
@petrklapka petrklapka removed the 1 label Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Unit Testing Project:i18n Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants