Remove injectI18n in dashboard plugin. by sainthkh · Pull Request #44580 · elastic/kibana

sainthkh · 2019-09-02T02:05:11Z

Summary

Inspired by #44043. Related to #38243.

Update to recommended usage of localization, using i18n.translate instead of injected intl prop in Dashboard app.

According to the I18n doc, it is recommended to avoid injectI18n and use i18n.translate instead.

Thoughts & Questions

1.
FormattedMessage requires I18nProvider as an ancestor to work correctly. If not, you'll see the error message below:

console.error packages/kbn-i18n/node_modules/react-intl/lib/index.js:1533
    [React Intl] Could not find required `intl` object. <IntlProvider> needs to exist in the component ancestry. Using default message as fallback.

To solve this problem, I've introduced shallowWithI18nProvider and mountWithI18nProvider in enzyme_helpers.tsx.

In the new platform, we'll need to unit test React components that have FormattedMessage but without I18nProvider.

Without these 2 functions, you cannot test them without seeing the error message above.

I think it would be a good idea to write documentation about these functions in the I18n doc. What do you think?

2.

In DashboardSaveModal in save_modal.tsx, <FormatMessage> component is used for component attributes. How about changing them to i18n.translate()?

I think i18n.translate is visually better.

I left them as-is because it works well without any problem and I have to change the test snapshot.

3.

CloneDashboardModal and DashboardSaveModal => Names are a bit inconsistent. Should we leave them as-is? If not, I think DashboardSaveModal to SaveDashboardModal is a good idea.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
--> Question about this is added above.
Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

@mattkime @timroes It seems that you're trying to move Dashboard to new platform. Can I get some review from you? Thanks in advance.

elasticmachine · 2019-09-02T02:05:13Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

mattkime · 2019-09-03T14:34:00Z

retest

elasticmachine · 2019-09-03T16:05:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

mattkime · 2019-09-03T20:56:21Z

hello @sainthkh - Thanks for the PR. (removed mistaken comment)

stacey-gammon · 2019-09-03T21:04:25Z

This is great @sainthkh! I saw @mattkime 's comment above and just wanted to say that I think FormattedMessage is appropriate to use (I was the PR author of the one you linked to, so I just went through this investigation).

In https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/GUIDELINE.md#in-reactjs

You should use <FormattedMessage> most of the time.

I'll try to take a look at the PR tomorrow. Just wanted to throw that in before you removed all usage of it!

stacey-gammon · 2019-09-03T21:07:06Z

cc @Bamieh to comment on suggested documentation changed.

CloneDashboardModal and DashboardSaveModal => Names are a bit inconsistent. Should we leave them as-is? If not, I think DashboardSaveModal to SaveDashboardModal is a good idea.

++ good idea, but I would not make it part of this PR.

Sorry for the drive by commenting, but gotta run soon!

sainthkh · 2019-09-03T22:55:08Z

It seems that Jest Integration tests failed because of timeout, not my changes.

mattkime · 2019-09-04T00:28:40Z

retest

sainthkh · 2019-09-04T00:57:51Z

I was a bit hasty. It started normal.

elasticmachine · 2019-09-04T01:58:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

mattkime · 2019-09-04T02:08:14Z

src/test_utils/public/enzyme_helpers.tsx


 export const nextTick = () => new Promise(res => process.nextTick(res));
+
+export function shallowWithI18nProvider<T>(child: ReactElement<T>) {


Did you try using this? https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/GUIDELINE.md#unit-tests

If so and it didn't work, lets talk about it. I want to make sure our reasoning is solid before adding new enzyme helpers.

It seems that the doc you're talking about is about using shallowWithIntl() or mountWithIntl() when injectI18n HOC is used.

We cannot use them in this PR, because I removed them. And it is recommended to do so in the i18n doc.

As far as I know, we need withI18nProvider functions to avoid the error message.

And as you can see here in the commit, WrappedComponents are gone.

I think we should update https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/GUIDELINE.md#unit-tests so it doesn't use props.intl in the example. I think @sainthkh's approach may be the right one, as long as there is a FormattedMessage somewhere in the line, then it'll need the provider wrapper.

…ove-injectI18n

stacey-gammon · 2019-09-05T16:16:46Z

retest

elasticmachine · 2019-09-05T17:35:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Removed injectI18n from dashboard listing * Added helpers for I18nProvider * Remove intl from DashboardCloneModal * Remove intl from options.tsx * Remove intl from DashboardSaveModal

stacey-gammon · 2019-09-05T17:50:13Z

backport PR : #44919

* Removed injectI18n from dashboard listing * Added helpers for I18nProvider * Remove intl from DashboardCloneModal * Remove intl from options.tsx * Remove intl from DashboardSaveModal

…ete-for-distance_feature * 'master' of github.com:elastic/kibana: (89 commits) Replace TSVB timeseries charts with elastic-charts (elastic#33558) [TSVB][Top N aggregation] Unable to deal with negative values (elastic#43581) [alerting] Adds Action Type configuration support and whitelisting (elastic#44483) FTR: fix WebDriver Actions calls (elastic#44605) [Code] add NodeRepositoriesService to watch new repositories on local node (elastic#44677) [skip-ci][Maps] Improve Maps intro page (elastic#44721) [Maps] Update titles and descriptions for data sources (elastic#44833) Types + Extract Integration Util (elastic#44433) Downgrade log level from info to debug for cases when we cannot handle authentication attempt. (elastic#44933) [Reporting] Remove Chome stdout/stderr observables, Add Browser Logger observable (elastic#44359) Update Jest script to output coverage (elastic#44447) [ftr] support --kibana-install-dir flag (elastic#44552) [WATCHER] Allow user to set a threshold value of 0 (elastic#44810) Remove injectI18n in dashboard plugin. (elastic#44580) [Graph] Save modal (elastic#44261) Use external script for the OIDC Implicit flow handler page. (elastic#44866) disable router prefixing with pluginId (elastic#44855) [SIEM] Fix bug on url + inspect functionality on hosts/hostDetails page (elastic#44671) [ML] File data viz limiting uploaded doc chunk size (elastic#44768) [code] Append go env variable 'GOCACHE' to go lsp spawn command. (elastic#44864) ...

…plate * 'master' of github.com:elastic/kibana: (91 commits) [APM] Make number of x ticks responsive to the plot width (elastic#44870) [ML] Single metric viewer: Fix top nav refresh behaviour. (elastic#44860) Replace TSVB timeseries charts with elastic-charts (elastic#33558) [TSVB][Top N aggregation] Unable to deal with negative values (elastic#43581) [alerting] Adds Action Type configuration support and whitelisting (elastic#44483) FTR: fix WebDriver Actions calls (elastic#44605) [Code] add NodeRepositoriesService to watch new repositories on local node (elastic#44677) [skip-ci][Maps] Improve Maps intro page (elastic#44721) [Maps] Update titles and descriptions for data sources (elastic#44833) Types + Extract Integration Util (elastic#44433) Downgrade log level from info to debug for cases when we cannot handle authentication attempt. (elastic#44933) [Reporting] Remove Chome stdout/stderr observables, Add Browser Logger observable (elastic#44359) Update Jest script to output coverage (elastic#44447) [ftr] support --kibana-install-dir flag (elastic#44552) [WATCHER] Allow user to set a threshold value of 0 (elastic#44810) Remove injectI18n in dashboard plugin. (elastic#44580) [Graph] Save modal (elastic#44261) Use external script for the OIDC Implicit flow handler page. (elastic#44866) disable router prefixing with pluginId (elastic#44855) [SIEM] Fix bug on url + inspect functionality on hosts/hostDetails page (elastic#44671) ...

sainthkh added 5 commits September 2, 2019 06:08

Removed injectI18n from dashboard listing

1b376ae

Added helpers for I18nProvider

6c38f57

Remove intl from DashboardCloneModal

ccd1b35

Remove intl from options.tsx

9c01d38

Remove intl from DashboardSaveModal

5c5d77d

mattkime self-requested a review September 3, 2019 14:24

mattkime added non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes v7.5.0 labels Sep 4, 2019

mattkime requested changes Sep 4, 2019

View reviewed changes

stacey-gammon approved these changes Sep 4, 2019

View reviewed changes

mattkime approved these changes Sep 4, 2019

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into dashboard-rem…

d36b645

…ove-injectI18n

stacey-gammon merged commit 8c44e89 into elastic:master Sep 5, 2019

stacey-gammon added v8.0.0 backport pending labels Sep 5, 2019

stacey-gammon mentioned this pull request Sep 5, 2019

[7.x] dashboard remove inject i18n #44919

Merged

This was referenced Sep 6, 2019

Writing documentation for i18n test helpers. #44975

Closed

Dashboard modal component name inconsistency. #44977

Closed

sainthkh mentioned this pull request Sep 20, 2019

Add i18n test helper documentation. #46298

Merged

2 tasks


		export const nextTick = () => new Promise(res => process.nextTick(res));

		export function shallowWithI18nProvider<T>(child: ReactElement<T>) {

Conversation

sainthkh commented Sep 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Thoughts & Questions

Checklist

For maintainers

Uh oh!

elasticmachine commented Sep 2, 2019

Uh oh!

mattkime commented Sep 3, 2019

Uh oh!

elasticmachine commented Sep 3, 2019

💔 Build Failed

Uh oh!

mattkime commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stacey-gammon commented Sep 3, 2019

Uh oh!

stacey-gammon commented Sep 3, 2019

Uh oh!

sainthkh commented Sep 3, 2019

Uh oh!

mattkime commented Sep 4, 2019

Uh oh!

sainthkh commented Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 4, 2019

💚 Build Succeeded

Uh oh!

mattkime Sep 4, 2019

Choose a reason for hiding this comment

Uh oh!

sainthkh Sep 4, 2019

Choose a reason for hiding this comment

Uh oh!

sainthkh Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stacey-gammon Sep 4, 2019

Choose a reason for hiding this comment

Uh oh!

stacey-gammon commented Sep 5, 2019

Uh oh!

elasticmachine commented Sep 5, 2019

💚 Build Succeeded

Uh oh!

stacey-gammon commented Sep 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sainthkh commented Sep 2, 2019 •

edited

Loading

mattkime commented Sep 3, 2019 •

edited

Loading

sainthkh commented Sep 4, 2019 •

edited

Loading

sainthkh Sep 4, 2019 •

edited

Loading