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.

Sign up for GitHub

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

Jump to bottom

[CCR] i18n fixes #27006

Merged
merged 1 commit into from
Dec 17, 2018
Merged

[CCR] i18n fixes #27006

merged 1 commit into from
Dec 17, 2018

Conversation

cjcenizal
Copy link
Contributor

Btw, we usually recommend to call translate within a function so that it's not called at module import time when there is a chance that plugin translations aren't initialized yet. I think in this case it will work, but if you can wrap it into a function that is called only when it's needed it'd be better.

@azasypkin Is there any way we can change the way plugin translations are initialized to ensure they're available? If this is at all possible, I think it would be a huge DX win to solve the problem from that angle rather than require consumers change the way they write code to account for this race condition.

@cjcenizal cjcenizal added Feature:CCR and Remote Clusters Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more labels Dec 12, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@azasypkin
Copy link
Member

@azasypkin Is there any way we can change the way plugin translations are initialized to ensure they're available? If this is at all possible, I think it would be a huge DX win to solve the problem from that angle rather than require consumers change the way they write code to account for this race condition.

Yes, we're changing this for legacy plugins here: #26078, so no worries :)

But i18n support in new platform is still in flux, if i18n is passed as service via plugin life-cycle hook arguments (as any other services in NP) then one wouldn't be able to just statically import it. We'll throw some ideas later and ask for feedback if it's going to change how we use i18n.

I may not be able to review PR earlier than the next week, so adding @Bamieh in case his availability is better than mine this week.

CI is failing because of inappropriate localizable label prefix 😉

@azasypkin azasypkin requested review from Bamieh and removed request for azasypkin December 12, 2018 08:34
@sebelga
Copy link
Contributor

sebelga commented Dec 12, 2018

@cjcenizal I am a bit worried about this PR as it touches almost all the components with lots of changes for just code style. I would have waited to have the feature completed (full CRUD) before making the code style recommendations. Please don't merge this before my DELETE PR 😄

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Bamieh
Bamieh reviewed Dec 13, 2018
View reviewed changes
x-pack/plugins/remote_clusters/public/sections/remote_cluster_form/remote_cluster_form.js Outdated
const newFields = {
...prevFields,
...changedFields,
this.setState({
Copy link
Member

@Bamieh Bamieh Dec 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

wouldn't it be better here to use this.setState(prevState => ({...}))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! This is being fixed in a separate PR.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cjcenizal
Copy link
Contributor Author

Retest

@cjcenizal cjcenizal mentioned this pull request Dec 14, 2018
Merged
3 tasks
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

sebelga
sebelga approved these changes Dec 17, 2018
View reviewed changes
Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

LGTM!

@cjcenizal
Basic i18n fixes.
743adb8 
- Add crossClusterReplication and remoteClusters to .i18nrc.json
- Replace snake case IDs with camel case.
- Export single components.
- Pass i18n check.
- Rename autofollow to autoFollow.
3
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cjcenizal cjcenizal merged commit f0da0f0 into elastic:feature/ccr Dec 17, 2018
@cjcenizal cjcenizal deleted the ccr/i18n-fixes branch December 17, 2018 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bamieh Bamieh Bamieh approved these changes

@sebelga sebelga sebelga approved these changes

Assignees
No one assigned
Labels
Feature:CCR and Remote Clusters Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cjcenizal @elasticmachine @azasypkin @sebelga @Bamieh