Fixes #17263 - move jed (i18n) to webpack #5342

amirfefer · 2018-03-21T14:22:03Z

No description provided.

theforeman-bot · 2018-03-21T14:22:15Z

sharvit

Thanks @amirfefer! love seeing staff moving into the webpack stack.

sharvit · 2018-03-21T15:23:13Z

webpack/assets/javascripts/bundle.js

@@ -19,6 +19,7 @@ require('./bundle_datatables');
 require('./bundle_lodash');

 import compute from './foreman_compute_resource';
+import i18n from './foreman_jed';


Should we name it as foreman_i18n.js?

sharvit · 2018-03-21T15:29:05Z

webpack/assets/javascripts/foreman_jed.js

+    return i18n;
+  },
+});
+Object.defineProperty(window, 'Jed', {


Does it make sense to add a new function to foreman_tools call deprecateObjectProperty?

export const deprecateObjectProperty = (obj, oldProp, newProp, version = '1.19') => Object.defineProperty(obj, oldProp, { get: () => { deprecate(oldProp, newProp, version); return obj[oldProp]; }, }); deprecateObjectProperty(window, 'i18n', 'tfm.i18n'); deprecateObjectProperty(window, 'Jed', 'tfm.i18n');

tstrachota

Thank you @amirfefer, it works nicely. I have only couple of comments in the code.

And then I have one special request. There's a concurrent PR, where I'm adding some more i18n functionality, which is mostly complementary, but it touches the same area so there's going to be name clashes: #5184
It's been there for some time and currently it's blocked by broken tests, but we know that we want to merge it one day. Therefore I wanted to ask if you could make sure that both things can be merged with minimal conflicts one day. That means it should ideally use the same files and already use the same functions for the common functionality like fetching locale codes.
The most conflicting part is probably: https://github.com/theforeman/foreman/pull/5184/files#diff-1551b3b23e104a39b3f69746fe110eae
I'll be happy to help you with this.

tstrachota · 2018-03-23T18:14:59Z

webpack/assets/javascripts/foreman_i18n.js

+
+window.__ = t_;
+window.n__ = (...args) =>
+  `${cheveronPrefix} ${i18n.ngettext(...args)} ${cheveronSuffix}`;


This one should imho be exported as well.

tstrachota · 2018-03-23T18:17:03Z

webpack/assets/javascripts/foreman_i18n.js

+const cheveronPrefix = window.I18N_MARK ? '\u00BB' : '';
+const cheveronSuffix = window.I18N_MARK ? '\u00AB' : '';
+export const t_ = (...args) =>
+  `${cheveronPrefix} ${i18n.gettext(...args)} ${cheveronSuffix}`;


If we want to keep the wrapping blank spaces, they should be part of the prefix/suffix so that we don't pollute the translated strings when I18N_MARK is turned off.

tstrachota · 2018-03-23T18:29:36Z

webpack/assets/javascripts/foreman_i18n.js

+};
+
+const locales = window.locales || emptyLocales;
+const i18n = new Jed(locales[getLocale()]);


Could we export Jed as i18n.jed or something similar? I'd like to avoid exporting it as default, since there's a concurrent PR, where I'm adding some more functionality:
https://github.com/theforeman/foreman/pull/5184/files#diff-1551b3b23e104a39b3f69746fe110eae

ohadlevy · 2018-03-29T11:47:16Z

test foreman

ohadlevy · 2018-03-29T11:47:27Z

[test foreman]

sharvit · 2018-03-29T11:59:08Z

webpack/assets/javascripts/foreman_hostgroups.js

@@ -1,10 +1,11 @@
 import $ from 'jquery';
+import { t_ } from './foreman_i18n';


When i look at this file, does it make sense to call it translate?

import { translate } from './foreman_i18n'; const warningMessage = translate('Some Puppet Classes are unavailable in the selected environment');

Some consumers can still use it as

import { translate as __ } from './foreman_i18n'; // or import { translate as t_ } from './foreman_i18n';

ohadlevy · 2018-05-09T05:55:21Z

@amirfefer @tstrachota what is the status of this pr? thanks

ohadlevy · 2018-06-25T08:13:20Z

@amirfefer what is the status of this PR? I would love to merge it prior to 1.19 branch.

ohadlevy · 2018-07-08T13:52:19Z

@amirfefer bump!

amirfefer · 2018-07-26T14:58:21Z

There were more prioritized tasks, i'll continue work on this soon.

amirfefer · 2018-08-01T12:00:59Z

@tstrachota , @ohadlevy
PR rebased and updated.

amirfefer · 2018-10-04T11:57:39Z

@ohadlevy could you have another look please?

ohadlevy · 2018-10-04T12:18:05Z

please have a look at the tests, i believe you didn't update PaginationHelper.js file?

ohadlevy · 2018-10-04T13:29:34Z

webpack/assets/javascripts/foreman_tools.js

@@ -123,3 +124,14 @@ export function updateTable(element) {
  Turbolinks.visit(uri.toString());
  return false;
 }
+
+export function deprecateObjectProperty(obj, oldProp, newProp, version = '1.20') {


can you please add a test for this new function?

ohadlevy · 2018-10-04T13:30:03Z

webpack/assets/javascripts/react_app/common/I18n.js

+const locales = window.locales || emptyLocales;
+let locale = document.getElementsByTagName('html')[0].lang;
+
+locale = locale.length === 0 ? 'en' : locale.replace(/-/g, '_');


i think this has no tests (for other locales) does it make sense to add one?

ohadlevy · 2018-10-04T13:32:01Z

@amirfefer also note the test failures, they seems related?

ohadlevy · 2018-10-07T08:25:45Z

webpack/assets/javascripts/react_app/common/I18n.js

+
+export const { sprintf } = jed;
+
+const i18n = { translate, ngettext, jed };


this should include sprintf? (if i understand the errors correctly this is the cause...?)

ohadlevy · 2018-10-07T14:38:35Z

@amirfefer please rebase - thanks

amirfefer · 2018-10-08T09:46:27Z

@ohadlevy rebased

ohadlevy · 2018-10-08T10:17:12Z

@amirfefer still missing a test for https://coveralls.io/builds/19399473/source?filename=webpack/assets/javascripts/foreman_tools.js#L130 ?

ohadlevy · 2018-10-08T11:10:37Z

test failures not related, imho ready for merge, bonus points if you can get the missing test for depreciation.

ekohl

👍 from a packaging perspective

amirfefer · 2018-10-08T12:52:33Z

@ohadlevy I've added a test for deprecateObjectProperty

ohadlevy · 2018-10-08T13:40:28Z

since @ekohl already approved, I'll merge once tests are 💚

ohadlevy · 2018-10-08T14:04:19Z

test failures not relevant, thanks @amirfefer

theforeman-bot added Needs testing Not yet reviewed labels Mar 21, 2018

amirfefer force-pushed the 17263 branch 2 times, most recently from 02746ea to a6255e9 Compare March 21, 2018 15:09

sharvit suggested changes Mar 21, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Mar 21, 2018

amirfefer force-pushed the 17263 branch from a6255e9 to 1cdbcdd Compare March 21, 2018 16:40

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Mar 21, 2018

amirfefer force-pushed the 17263 branch from 1cdbcdd to 37f3c61 Compare March 22, 2018 11:42

tstrachota suggested changes Mar 23, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Mar 23, 2018

sharvit reviewed Mar 29, 2018

View reviewed changes

lizagilman mentioned this pull request Jul 10, 2018

Fixes #24011 - Add Patternfly bar chart support #5718

Merged

8 tasks

boaz0 mentioned this pull request Jul 26, 2018

Fixes #24475 - Add EmptyState to React components #5850

Merged

amirfefer force-pushed the 17263 branch from 37f3c61 to cd09e97 Compare August 1, 2018 11:59

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Aug 1, 2018

amirfefer force-pushed the 17263 branch from 77a6afa to fd76a01 Compare October 4, 2018 11:49

theforeman-bot added Needs re-review and removed Waiting on contributor labels Oct 4, 2018

amirfefer force-pushed the 17263 branch from fd76a01 to d5870bf Compare October 4, 2018 11:56

amirfefer force-pushed the 17263 branch from d5870bf to f99e51b Compare October 4, 2018 12:39

ohadlevy reviewed Oct 4, 2018

View reviewed changes

ohadlevy reviewed Oct 7, 2018

View reviewed changes

amirfefer force-pushed the 17263 branch from f99e51b to c6a6b22 Compare October 7, 2018 12:31

amirfefer mentioned this pull request Oct 7, 2018

Fixes #25135 - use i18n from webpack Katello/katello#7739

Merged

amirfefer force-pushed the 17263 branch from c6a6b22 to 2dd40c9 Compare October 8, 2018 08:42

ohadlevy mentioned this pull request Oct 8, 2018

Update foreman package dependencies theforeman/foreman-packaging#3070

Merged

ekohl previously approved these changes Oct 8, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Oct 8, 2018

amirfefer dismissed ekohl’s stale review via 419da30 October 8, 2018 12:51

amirfefer force-pushed the 17263 branch from 2dd40c9 to 419da30 Compare October 8, 2018 12:51

Fixes #17263 - move jed (i18n) to webpack

bd50311

amirfefer force-pushed the 17263 branch from 419da30 to bd50311 Compare October 8, 2018 13:11

ohadlevy approved these changes Oct 8, 2018

View reviewed changes

ohadlevy merged commit 60e4da4 into theforeman:develop Oct 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #17263 - move jed (i18n) to webpack #5342

Fixes #17263 - move jed (i18n) to webpack #5342

amirfefer commented Mar 21, 2018

theforeman-bot commented Mar 21, 2018

sharvit left a comment

sharvit Mar 21, 2018

sharvit Mar 21, 2018 •

edited

Loading

tstrachota left a comment

tstrachota Mar 23, 2018

tstrachota Mar 23, 2018

tstrachota Mar 23, 2018

ohadlevy commented Mar 29, 2018

ohadlevy commented Mar 29, 2018

sharvit Mar 29, 2018

ohadlevy commented May 9, 2018

ohadlevy commented Jun 25, 2018

ohadlevy commented Jul 8, 2018

amirfefer commented Jul 26, 2018 •

edited

Loading

amirfefer commented Aug 1, 2018

amirfefer commented Oct 4, 2018

ohadlevy commented Oct 4, 2018

ohadlevy Oct 4, 2018

ohadlevy Oct 4, 2018

ohadlevy commented Oct 4, 2018

ohadlevy Oct 7, 2018

ohadlevy commented Oct 7, 2018

amirfefer commented Oct 8, 2018

ohadlevy commented Oct 8, 2018

ohadlevy commented Oct 8, 2018

ekohl left a comment

amirfefer commented Oct 8, 2018

ohadlevy commented Oct 8, 2018

ohadlevy commented Oct 8, 2018

		@@ -1,10 +1,11 @@
		import $ from 'jquery';
		import { t_ } from './foreman_i18n';


		export const { sprintf } = jed;

		const i18n = { translate, ngettext, jed };

Fixes #17263 - move jed (i18n) to webpack #5342

Fixes #17263 - move jed (i18n) to webpack #5342

Conversation

amirfefer commented Mar 21, 2018

theforeman-bot commented Mar 21, 2018

sharvit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharvit Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

tstrachota left a 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

ohadlevy commented Mar 29, 2018

ohadlevy commented Mar 29, 2018

Choose a reason for hiding this comment

ohadlevy commented May 9, 2018

ohadlevy commented Jun 25, 2018

ohadlevy commented Jul 8, 2018

amirfefer commented Jul 26, 2018 • edited Loading

amirfefer commented Aug 1, 2018

amirfefer commented Oct 4, 2018

ohadlevy commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohadlevy commented Oct 4, 2018

Choose a reason for hiding this comment

ohadlevy commented Oct 7, 2018

amirfefer commented Oct 8, 2018

ohadlevy commented Oct 8, 2018

ohadlevy commented Oct 8, 2018

ekohl left a comment

Choose a reason for hiding this comment

amirfefer commented Oct 8, 2018

ohadlevy commented Oct 8, 2018

ohadlevy commented Oct 8, 2018

sharvit Mar 21, 2018 •

edited

Loading

amirfefer commented Jul 26, 2018 •

edited

Loading