[EuiSuperDatePicker] i18n all remaining static copy + convert prettyDuration to hook/component #5743

cee-chen · 2022-03-23T19:33:19Z

Summary

closes #5398

Screencaps

Now tab Before/After

Relative units dropdown before/after

Absolute tab before/after (note that I switched the locale to ja in the after screenshot for testing)

Quick select before/after

Refresh interval before/after

Review

I strongly recommend following along by commit - it gets particularly gnarly after the 5th commit. I tried my best to keep things atomic, but this is an inherently difficult PR to grok due to the nature of the changes that had to be made.

I'm going to quote a Slack conversation I had with @thompsongl that captured why these changes were so difficult and why this approach was the least worst of all worlds:

We've got a pretty gnarly tangle of un-i18n'd string interpolation, the problem being that we have them in top-level exported constants being used interchangeably in both vanilla JS util functions (which can't have hooks or react context) and React components. An extra snag is that most of the super date picker components are old school class components, meaning they can't use useEuiI18n hooks. At this point I think my options are:

Convert everything (including vanilla util fns) into custom use hooks so they can call useEuiI18n and all class components into functional components that can use hooks

Somehow magically figure out a helper that allows us to use our i18n helpers in vanilla JS outside of a React component or hook

Set a top level render component that passes down the i18n'd strings/arrays/objects as a constant and waterfall those i18n'd constants as props/args to every use of them (extremely verbose but probably the least technically unknown)

Another snag: we're exporting prettyDuration, a vanilla JS util that contain un-i18n'd strings. There's simply no way I can see to leave prettyDuration as a util fn and not convert it into a hook and still contain i18n'd strings.

Greg Thompson
If we want to get this in for 8.2, option 3 is definitely the safest.
Not sure option 2 is even possible because the i18n stuff is all based on React context

Checklist

~~- [ ] Checked in both light and dark modes~~
- [ ] Checked in mobile
~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
~~- [ ] Updated the Figma library counterpart~~

Checked in Chrome, Safari, Edge, and Firefox
Props have proper autodocs and playground toggles
Added or updated jest and cypress tests
Updated documentation
Checked for breaking changes and labeled appropriately
A changelog entry exists and is marked appropriately
Revert [REVERT] commit

- Example setup of how we'll be handling all other non-i18n'd constants used throughout datepicker

…ePicker + update external/internal required props accordingly - there were a lot of props marked required that weren't actually required because they had defaults

…ed it

- it has no i18n strings and can be a constant

+ refactor prettyInterval structure with `useI18nUnits` helper - because hooks can't be called conditionally, I had to rewrite a good amount to try and keep the code readable

- This one was tricky and what necessitated the render prop wrapper, because commonlyUsedRanges uses it as a default prop - this was the most straightforward way I could think of to pass it as a fallback

- cannot be a vanilla JS util any longer if it's using i18n (which requires React context) - a hook+component should covert both cases where a string is required vs rendering JSX - reorder utils slightly - update formatTimeString to hook as well to render i18n strings

thompsongl

A couple more imports to update:

WARNING in ./views/pretty_duration/pretty_duration_example.js 21:56-76
"export 'commonDurationRanges' was not found in '../../../../src/components'

WARNING in ./views/pretty_duration/pretty_duration.js 56:62-76
"export 'prettyDuration' was not found in '../../../../src/components'

src/components/date_picker/super_date_picker/time_options.tsx

cee-chen · 2022-03-23T20:18:47Z

Doh, I totally missed that we had a documentation page for prettyDuration 🤦‍♀️ on it!

kibanamachine · 2022-03-23T20:52:28Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5743/

…i into i18n-super-date-picker

cee-chen · 2022-03-23T22:16:17Z

I can confirm this works locally despite the typing:

Casting as works but I'm not sure if there's a more elegant way of overriding Action<>'s getDisplayName definition 🤔

thompsongl · 2022-03-23T22:34:06Z

Casting as works but I'm not sure if there's a more elegant way of overriding Action<>'s getDisplayName definition

I think we should 1) cast and 2) check with the Action owners about expanding the type to ReactNode or something more accommodating

Bamieh · 2022-03-24T10:30:44Z

src/components/date_picker/super_date_picker/time_options.tsx

+      value: 'y',
+    },
+    {
+      text: useEuiI18n('euiTimeOptions.secondsFromNow', 'Seconds from now'),


These values will show on the UI with a time unit next to them, right? eg 5 Seconds from now

If that is the case then Ideally we'd want to pass the time unit as a variable to the i18n message:

{unitAmount} Seconds from now

This way we can move the variable around depending on the language. In some languages, it doesn't make sense to have the number at the beginning of the text.

Will that require a lot of changes to implement?

Hey @Bamieh! These specific values are actually not shown in the UI with units in front of them, they are only shown in the below highlighted dropdown/EuiSelect:

The actual value that displays in the datepicker input (that toggles the popover) typically comes from Moment itself if it's not a quick range label (e.g. year to date), and moment should be handling the i18n of that via the locale prop.

Oh, actually, I hyperfocused on the line you selected and not on the general principal of what you were saying. There is a place in PrettyDuration where we concatenate time, time units, and 'last/next' together. I'll flag that and move our convo there if that's cool

Yes, the ~30分前 in the screenshot above is from moment not from EUI's i18n strings.

cee-chen · 2022-03-24T15:28:52Z

src/components/date_picker/super_date_picker/pretty_duration.tsx

+        ? timeUnitsPlural[countTimeUnit]
+        : timeUnits[countTimeUnit];
+
+    relativeDuration = `${timeTense} ${relativeParts.count} ${countTimeUnitFullName}`;


Re-tagging @Bamieh on this discussion:

Ideally we'd want to pass the time unit as a variable to the i18n message:
{unitAmount} Seconds from now
This way we can move the variable around depending on the language. In some languages, it doesn't make sense to have the number at the beginning of the text.

This is the primary place I can think of where we concatenate something like that. This string outputs something like Last 30 minutes or Next 1 hour.

@Bamieh just to make sure I'm understanding you, you would rather see this as a series of i18n tokens, is that correct? Would something like

Last {timeAmount} {timeUnit} Next {timeAmount} {timeUnit}

work or do you need it to be more granular and have us implement something like last Last {timeAmount} second(s) (so a token for each unit type and pluralization?... crossing my fingers that you don't need this as that would be a really high amount of tokens 😬)

I also want to quickly flag that we are using some of these minutes/seconds tokens as-is with no time amount in front of them for our dropdown selections. So we were essentially trying to use concatenation to avoid doubling the amount of tokens (for the dropdowns and the output). But you're saying we should be doubling up for better translation, correct?

Syncing on slack to reduce the amount of back and forth inside the PR. Will reflect the resolution here on github once we reach it

cee-chen · 2022-03-24T15:31:32Z

src/components/date_picker/super_date_picker/pretty_interval.ts

-  if (shortHand) return `${intervalInDays} d`;
-  units = intervalInDays > 1 ? 'days' : 'day';
-  return `${intervalInDays} ${units}`;
+  return `${interval} ${units}`;


@Bamieh Also wanted to flag this pretty interval concatenation. This is meant to output things like: 30 minutes, 1 minute, 1 m. Are you saying we should also have a token for each unit type? i.e., one for {timeUnit} minute(s) and one for {timeUnit} m?

Per i18n team feedback, we shouldn't be chopping units up like this and concatenating them manually - we should prefer to attempt to tokenize whole sentences to better accommodate the different grammars of different languages + add more misc comment documentation + capitalize units in timeUnitOptions to match relativeOptions

- preferring to let languages translate each phrase per unit

- allows consuming applications to determine grammar/number placement, pluralization, etc

- allows consuming applications to determine grammar/number placement, pluralization, etc - pretty_duration has its own custom i18n strings not shared with other parts of EuiDatePicker, so I put them at the top of the file instead of in time_options + refactored/greatly simplified relativeDuration logic - a lot of the necessary keys were already in relativeParts, no need to substr, get timeTense, etc - structure is still somewhat weird due to not being able to call hooks conditionally + DRY out TimeUnitAllId for string maps with both `+` and `-` time units + add comment docblocks for readability/organization

- <EuiI18n> with `tokens` arrays was not passing an i18nMappingFunc - i18n tokens that passed a function were not passing the rendered function output to the i18nMappingFunc - Updated the i18n documentation page to show both of these examples now working with the babelfish toggled on

cee-chen · 2022-03-24T22:57:26Z

@Bamieh Alrighty, I should have pushed up all i18n token changes that I think follow the best practices you've outlined. Please let me know if it would be easier for you if I listed out all the tokens in a text file or similar instead of you having to parse through my commits/diffs.

@thompsongl the i18n babelfish issues we discussed in Slack are in d53c0a2. Honestly I have no idea what on earth is going on with the typing in that file so I just cast a bunch and crossed my fingers. It does appear to be working per my dev environment testing, although it also makes me wonder why the unit tests never caught the issue 😅 I'm fully open to reverting the commit in this PR before it merges and opening a follow-up PR with the fix + perhaps more robust unit tests.

- moved in latest main

kibanamachine · 2022-03-24T23:30:27Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5743/

Bamieh

LGTM thank you!

src-docs/src/views/super_date_picker/super_date_picker.tsx

src/components/i18n/i18n.tsx

thompsongl · 2022-03-28T14:37:53Z

.../date_picker/super_date_picker/quick_select_popover/__snapshots__/quick_select.test.tsx.snap

@@ -113,7 +113,7 @@ exports[`EuiQuickSelect is rendered 1`] = `
        values={
          Object {
            "timeTense": "last",
-            "timeUnit": "minutes",
+            "timeUnit": "Minutes",


Just noting the casing change. Was there a specific reason for this? Seems fine to me regardless

I was trying to match our other dropdown selection in the relative tab that says Minutes ago, Seconds ago, etc. I also thought that capitalizing it would lowkey make it harder to concatenate with other strings in the future haha

This reverts commit d53c0a2.

This reverts commit f28d902.

kibanamachine · 2022-03-28T16:02:14Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5743/

thompsongl

LGTM!
I did another build with Kibana that resulted in the expected type changes

cee-chen · 2022-03-28T17:56:10Z

Awesome, thanks y'all. Is this good to merge now or do we need to wait for any Kibana/FF timing or anything?

cee-chen · 2022-03-28T18:22:40Z

Chandler has given the green light on this, so going to go ahead and merge + do a release in the next 10 minutes.

cee-chen added 12 commits March 23, 2022 11:09

i18n Absolute/Relative/Now tab labels

2987165

i18n + DRY start/end date input prepend labels

242391a

i18n Absolute date format error

53e4e62

i18n Now tab copy

861b5c5

Convert non-i18n'd time constants to hook with i18n

30614d5

- Example setup of how we'll be handling all other non-i18n'd constants used throughout datepicker

Add render function that passes i18n strings/constants to EuiSuperDat…

5def024

…ePicker + update external/internal required props accordingly - there were a lot of props marked required that weren't actually required because they had defaults

Waterfall time options constants as props to child components that ne…

94984f6

…ed it

i18n relative tab dropdown options

d876e4d

DRY out relativeUnitsFromLargestToSmallest

fd4050c

- it has no i18n strings and can be a constant

i18n refresh dropdown options

c18988b

i18n refresh shorthand units / convert prettyInterval to hook

2709cc0

+ refactor prettyInterval structure with `useI18nUnits` helper - because hooks can't be called conditionally, I had to rewrite a good amount to try and keep the code readable

i18n Commonly used ranges in quick select

c08fc93

- This one was tricky and what necessitated the render prop wrapper, because commonlyUsedRanges uses it as a default prop - this was the most straightforward way I could think of to pass it as a fallback

cee-chen added breaking change !urgent! labels Mar 23, 2022

cee-chen added this to the Elastic Stack 8.2 milestone Mar 23, 2022

cee-chen changed the title ~~I18n super date picker~~ [EuiSuperDatePicker] i18n all remaining static copy + convert prettyDuration to hook/component Mar 23, 2022

cee-chen added 5 commits March 23, 2022 12:43

Add time_options unit tests

94ebca3

Update various snapshot/unit tests with timeOptions waterfalling

02198dd

[REVERT] Test moment locale

f28d902

Add changelog entry

d4fb250

cee-chen force-pushed the i18n-super-date-picker branch from 35543c1 to d4fb250 Compare March 23, 2022 19:43

thompsongl reviewed Mar 23, 2022

View reviewed changes

src/components/date_picker/super_date_picker/time_options.tsx Outdated Show resolved Hide resolved

cee-chen commented Mar 23, 2022

View reviewed changes

src/components/date_picker/super_date_picker/time_options.tsx Outdated Show resolved Hide resolved

derpin

2bfd682

cee-chen added 2 commits March 23, 2022 13:55

Update Pretty duration docs

b837a56

Merge branch 'i18n-super-date-picker' of github.com:constancecchen/eu…

2bda1db

…i into i18n-super-date-picker

Bamieh reviewed Mar 24, 2022

View reviewed changes

cee-chen commented Mar 24, 2022

View reviewed changes

cee-chen added 3 commits March 24, 2022 14:46

[i18n feedback] Remove concatenated unit string in relative tab

e933241

- preferring to let languages translate each phrase per unit

[i18n feedback] Remove concatenated unit strings in pretty_interval

28c937f

- allows consuming applications to determine grammar/number placement, pluralization, etc

cee-chen requested a review from Bamieh March 24, 2022 22:13

cee-chen added 3 commits March 24, 2022 15:52

Update snapshots

10792e7

cee-chen force-pushed the i18n-super-date-picker branch from 420e906 to d53c0a2 Compare March 24, 2022 22:52

cee-chen added 2 commits March 24, 2022 15:59

Merge branch 'main' into i18n-super-date-picker

acf40f4

Fix testCustomHook move

238686c

- moved in latest main

Bamieh approved these changes Mar 28, 2022

View reviewed changes

thompsongl reviewed Mar 28, 2022

View reviewed changes

cee-chen added 2 commits March 28, 2022 08:19

Revert "Fix broken i18n behavior"

1dc826d

This reverts commit d53c0a2.

Revert "[REVERT] Test moment locale"

6bc7035

This reverts commit f28d902.

cee-chen mentioned this pull request Mar 28, 2022

[EuiI18n] Fixed EuiContext.i18n's mappingFunc not being called for EuiI18n with multiple tokens and function callbacks #5748

Merged

3 tasks

thompsongl approved these changes Mar 28, 2022

View reviewed changes

cee-chen merged commit 037c4ec into elastic:main Mar 28, 2022

cee-chen deleted the i18n-super-date-picker branch March 28, 2022 18:33

cee-chen mentioned this pull request Mar 29, 2022

Upgrade EUI to v53.0.1 elastic/kibana#128825

Merged

thompsongl mentioned this pull request Apr 11, 2022

[EuiSuperDatePicker] Date formatting and i18n #5266

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiSuperDatePicker] i18n all remaining static copy + convert prettyDuration to hook/component #5743

[EuiSuperDatePicker] i18n all remaining static copy + convert prettyDuration to hook/component #5743

cee-chen commented Mar 23, 2022 •

edited

Loading

thompsongl left a comment

cee-chen commented Mar 23, 2022

kibanamachine commented Mar 23, 2022

cee-chen commented Mar 23, 2022

thompsongl commented Mar 23, 2022

Bamieh Mar 24, 2022

cee-chen Mar 24, 2022 •

edited

Loading

cee-chen Mar 24, 2022

thompsongl Mar 24, 2022 •

edited

Loading

cee-chen Mar 24, 2022

cee-chen Mar 24, 2022

Bamieh Mar 24, 2022

cee-chen Mar 24, 2022

cee-chen commented Mar 24, 2022

kibanamachine commented Mar 24, 2022

Bamieh left a comment

thompsongl Mar 28, 2022

cee-chen Mar 28, 2022

kibanamachine commented Mar 28, 2022

thompsongl left a comment

cee-chen commented Mar 28, 2022

cee-chen commented Mar 28, 2022

[EuiSuperDatePicker] i18n all remaining static copy + convert prettyDuration to hook/component #5743

[EuiSuperDatePicker] i18n all remaining static copy + convert prettyDuration to hook/component #5743

Conversation

cee-chen commented Mar 23, 2022 • edited Loading

Summary

Screencaps

Review

Checklist

thompsongl left a comment

Choose a reason for hiding this comment

cee-chen commented Mar 23, 2022

kibanamachine commented Mar 23, 2022

cee-chen commented Mar 23, 2022

thompsongl commented Mar 23, 2022

Choose a reason for hiding this comment

cee-chen Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thompsongl Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen commented Mar 24, 2022

kibanamachine commented Mar 24, 2022

Bamieh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Mar 28, 2022

thompsongl left a comment

Choose a reason for hiding this comment

cee-chen commented Mar 28, 2022

cee-chen commented Mar 28, 2022

cee-chen commented Mar 23, 2022 •

edited

Loading

cee-chen Mar 24, 2022 •

edited

Loading

thompsongl Mar 24, 2022 •

edited

Loading