-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[i18n] Add tests to @elastic/eui tokens #99613
Conversation
/** | ||
* List here those EUI tokens which defaultMessages are intentionally changed in Kibana. | ||
* Looking at the examples in the list, it looks like this is a workaround | ||
* while @elastic/eui fixes the translations on their end. | ||
*/ | ||
const INTENTIONALLY_OVERRIDDEN_TRANSLATIONS: EuiToken[] = [ | ||
'euiTourStep.closeTour', // "Close tour" makes more sense for consistency, since we have "Skip tour" and "End tour" | ||
'euiTourStepIndicator.ariaLabel', // `@elastic/eui` extracted it as a stringified function | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed many inconsistencies in Kibana to match @elastic/eui
's guidance. But these 2 keys, I actually think they make more sense how Kibana defines them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc'ing @ryankeairns since git blame points to him as the dev that added those two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential fix for euiTourStepIndicator.ariaLabel
elastic/eui#4785
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also created the discuss issue elastic/eui#4787 for euiTourStep.closeTour
defaultMessage: '${count} ${filterCountLabel} filters', | ||
values: { count, filterCountLabel: hasActiveFilters ? 'active' : 'available' }, | ||
defaultMessage: '{count} {hasActiveFilters} filters', | ||
values: { count, hasActiveFilters: hasActiveFilters ? 'active' : 'available' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be doing that 'active' | 'available'
text replacement here. They'll show as-is for any other locales.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #99644 to follow that fix.
@elasticmachine merge upstream |
merge conflict between base and head |
51a9250
to
d0811bc
Compare
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / jest / Jest Tests.x-pack/plugins/apm/public/components/shared/TransactionActionMenu.TransactionActionMenu component matches the snapshotStandard Out
Stack Trace
Kibana Pipeline / jest / Jest Tests.x-pack/plugins/reporting/public/components.ReportListing Report job listing with some itemsStandard Out
Stack Trace
Kibana Pipeline / jest / Jest Tests.x-pack/plugins/index_lifecycle_management/__jest__.extend index management ilm summary extension should return extension when index has lifecycle errorStandard Out
Stack Trace
and 133 more failures, only showing the first 3. Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Closing in favor of #106377 |
Summary
Add tests to ensure that the i18n tokens from
@elastic/eui
and their mapping in Kibana are in sync.The tests are meant to apply the following checks:
core.{EUI Tokens}
?defaultValue
match EUI'sdefString
for every token?values
as expected in the props ofEuiI18n
?Related issues/PRs
Checklist
Delete any items that are not applicable to this PR.
For maintainers