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 angular-translate #13066

Merged
merged 3 commits into from
Jan 12, 2018
Merged

Remove angular-translate #13066

merged 3 commits into from
Jan 12, 2018

Conversation

epixa
Copy link
Contributor

@epixa epixa commented Jul 24, 2017

We're not abandoning the translation effort altogether, but we're
rolling back the angular-translate integration specifically as it has
too great of a negative effect on the dev experience. We'll revisit
template interpolation using a more component-oriented design.

This doc file was never actually included in docs in the first place,
so I just removed it rather than trying to rewrite it.

@epixa
Copy link
Contributor Author

epixa commented Jul 24, 2017

@Bargs Here's the followup from #13049

@spalger
Copy link
Contributor

spalger commented Jul 25, 2017

jenkins, test this

@spalger
Copy link
Contributor

spalger commented Jul 25, 2017

The i18nextract task, which is what creates the build/i18n_extract/en.json file mentioned in tasks/build/verify_translations.js is from grunt-angular-translate, so config/i18nextract.js and parts of tasks/build/verify_translations.js should be removed too.

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

A couple things to remove, but assuming the test failures are not caused by this LGTM

@spalger
Copy link
Contributor

spalger commented Jul 25, 2017

Same failure, must be related to these changed

@epixa
Copy link
Contributor Author

epixa commented Jul 26, 2017

Great...

@epixa epixa added v5.6.1 and removed v5.6.0 labels Sep 7, 2017
@jimgoodwin jimgoodwin added v5.6.2 and removed v5.6.1 labels Sep 18, 2017
@epixa epixa added v5.6.4 and removed v5.6.3 labels Oct 11, 2017
@Bargs Bargs removed their request for review October 30, 2017 23:02
@epixa epixa added v5.6.5 and removed v5.6.4 labels Nov 6, 2017
@epixa epixa added v6.0.1 and removed v6.0.0 labels Nov 13, 2017
@jimgoodwin jimgoodwin added v6.1.1 and removed v6.0.2 labels Dec 13, 2017
@epixa epixa added v6.1.2 and removed v6.1.1 labels Dec 17, 2017
@epixa epixa force-pushed the removetranslations branch from aed4b52 to d879689 Compare January 9, 2018 01:08
@epixa epixa added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc chore labels Jan 9, 2018
@epixa epixa force-pushed the removetranslations branch 4 times, most recently from 459c3db to 89becdc Compare January 10, 2018 16:17
@epixa
Copy link
Contributor Author

epixa commented Jan 10, 2018

This had a green build on the changes here before the yarn changes https://kibana-ci.elastic.co/job/elastic-kibana-pull-request/14573/

I pushed up a new commit that updates this PR for yarn.

@Bargs Bargs self-requested a review January 10, 2018 19:34
Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

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

Left one small comment which you can take or leave.

LGTM

@@ -16,9 +16,6 @@ function init() {

debounce = $injector.get('debounce');
debounceFromProvider = Private(DebounceProvider);

// ensure there is a clean slate before testing deferred tasks
$timeout.flush();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this needs to be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does need to be removed. I'll be honestly, I'm not sure why exactly, but something angular-translate was doing required this. Prior to removing this, the tests failed with an error related to there being nothing to flush, and since this line was added in the original PR and tests now pass without it, it seemed unnecessary. If this was not a test file, I would have dug into it more to understand what's going on, but it just didn't feel worth digging deep into since my goal here is essentially to undo that original PR anyway.

@epixa epixa merged commit 61c7f21 into elastic:master Jan 12, 2018
@epixa epixa deleted the removetranslations branch January 12, 2018 14:15
epixa added a commit to epixa/kibana that referenced this pull request Jan 12, 2018
* remove angular translate

* remove angular-translate deps from yarn.lock
epixa added a commit that referenced this pull request Jan 27, 2018
* remove angular translate

* remove angular-translate deps from yarn.lock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v6.2.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants