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

Remove injectI18n in management. #45876

Merged
merged 12 commits into from
Oct 8, 2019
Merged

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Sep 17, 2019

Summary

Inspired by #44043. Related to #38610.

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.

Notes

  • Most changes are simple: remove injectI18n, remove WrappedComponent, change intl to i18n.translate.
  • But as for Field.js, it wasn't simple. In jest test, Field.js uses setProps and it doesn't work with child node. It only works with the root node.. So, I had to remove all FormattedMessage from Field.js to use mount() function.
    • That's why that change is separated from other changes.

Checklist

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

For maintainers

Note

  • I've used regex below to remove intl fast:
intl\.formatMessage\(\{\n\s+id:\s+('.*'),\n*
i18n.translate($1, {
  • I wrote it here for my later use or for others who want to do the similar job.

@elasticmachine
Copy link
Contributor

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?

1 similar comment
@elasticmachine
Copy link
Contributor

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?

@sainthkh sainthkh marked this pull request as ready for review September 18, 2019 03:17
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@mattkime
Copy link
Contributor

mattkime commented Oct 3, 2019

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@mattkime
Copy link
Contributor

mattkime commented Oct 3, 2019

Looks like there are more changes to src/legacy/core_plugins/kibana/public/management/sections/settings/components/field/snapshots/field.test.js.snap than wanted.

@mattkime
Copy link
Contributor

mattkime commented Oct 3, 2019

It might be easier to break this PR up into a series of smaller PRs particularly due to the failing functional tests

@sainthkh sainthkh force-pushed the management-i18n branch 2 times, most recently from 7b27091 to bc909ac Compare October 3, 2019 21:35
@sainthkh sainthkh requested a review from a team October 3, 2019 23:34
@sainthkh
Copy link
Contributor Author

sainthkh commented Oct 3, 2019

  1. After checking things out, I found that the cause of all those functional test failures was "not changing the name of a component". After fixing this, all functional tests and eslint test passed.
  2. As setProps can be called only on the root instance (if not, an exception will be thrown), I removed all <FormattedMessage>s at that time. But after a while, I thought changing code for test code is not good. So, I decided to create wrapper component for tests.
  3. const updated is added to get the updated render results. Without them, test fails. I guess this happens because tested component is not a root component.

@mattkime
Copy link
Contributor

mattkime commented Oct 4, 2019

retest

@mattkime mattkime added release_note:skip Skip the PR/issue when compiling release notes v8.0.0 v7.5.0 labels Oct 4, 2019
@@ -21,7 +21,7 @@ import React from 'react';
import { shallow } from 'enzyme';
import { shallowWithI18nProvider } from 'test_utils/enzyme_helpers';

import { TableComponent } from '../table';
import { Table } from '../table';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@elasticmachine
Copy link
Contributor

💔 Build Failed

@mattkime
Copy link
Contributor

mattkime commented Oct 4, 2019

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

LGTM. thanks!

@mattkime
Copy link
Contributor

mattkime commented Oct 7, 2019

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@mattkime
Copy link
Contributor

mattkime commented Oct 8, 2019

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mattkime mattkime merged commit 8fd1a36 into elastic:master Oct 8, 2019
mattkime pushed a commit to mattkime/kibana that referenced this pull request Oct 8, 2019
* Remove injectI18n in management
mattkime added a commit that referenced this pull request Oct 8, 2019
* Remove injectI18n in management
cqliu1 pushed a commit to cqliu1/kibana that referenced this pull request Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants