fix The Korean sentence order in Authorship description by tandara0 · Pull Request #2947 · OHDSI/Atlas

tandara0 · 2024-07-31T02:20:23Z

This commit is about the issue: OHDSI/WebAPI#2381
And the commit involved in WebAPI: OHDSI/WebAPI#2382

Signed-off-by: Juyoung Kim <tandara0@gmail.com>

chrisknoll · 2024-08-02T14:26:29Z

Is there any way you can combine those multiple elements into one templated field like:

 "createdLabel": "<%=createdBy%>가  경에 생성 <%=createdDate%>",

Such that we create one element to contain the full sentence of what the created by message shoudl be (and separately, the modified one).

This is the code:

<span data-bind="text: createdText"></span>
	<span data-bind="visible: createdBy, text: ko.i18nformat('components.authorship.byCreated', 'by <%=createdBy%>', {createdBy: createdBy})"></span>
	<span data-bind="text: ko.i18n('components.authorship.on', 'on')"></span>
	<span data-bind="text: createdDate"></span>
	<span data-bind="text: ko.i18n('components.authorship.createdEnd', ' ')"></span><span data-bind="visible: modifiedDate">,</span>
	<span data-bind="visible: modifiedDate">
		<span data-bind="text: ko.i18n('components.authorship.modified', 'modified')"></span>
		<span data-bind="visible: createdBy, text: ko.i18nformat('components.authorship.byModified', 'by <%=modifiedBy%>', {modifiedBy: modifiedBy})"></span>
		<span data-bind="text: ko.i18n('components.authorship.on', 'on')"></span>
		<span data-bind="text: modifiedDate"></span>
		<span data-bind="text: ko.i18n('components.authorship.modifiedEnd', ' ')"></span>
	</span>

The issue is the content of the message is being broken apart by multiple spans and databinds (and multiple i18n tokens). Would it be possible to restructure this into just 2 spans with 2 i18n tokens (components.authorship.createdLabel and components.authorship.modifiedLabel), and allt he above spans turn into something like:

<span data-bind="visible: createdBy, 
  text: ko.i18nformat('components.authorship.createdLabel', 
                                  'by <%=createdBy%> on <%=cratedDate%>', 
                                  {createdBy: createdBy, createdDate: createdDate})"></span>
<span data-bind="visible: modifiedBy, 
  text: ko.i18nformat('components.authorship.modifiedLabel', 
                                  'by <%=modifiedBy%> on <%=modifiedDate%>', 
                                  {createdBy: createdBy, createdDate: createdDate})"></span>

This would solve the problem of language structure by encapsulating the entire sentence into one block.

Would this be possible for you to do? I think I'd prefer a solution that reduces the number of I18N tokens in our resources instead of adding new ones that only apply in certain contexts.

tandara0 · 2024-08-05T06:50:59Z

Dear @chrisknoll,

I agree to reduce the elements of the authorship components.
If I understand what you said, the authorship components of english will be changed like:

before:

    "authorship": {
      "created": "created",
      "versionCreated": "version created",
      "modified": "modified",
      "on": "on",
      "byCreated": "by <%=createdBy%>",
      "byModified": "by <%=modifiedBy%>"
    },

after:

    "authorship": {
      "createdLabel": "created by <%=createdBy%> on <%=createdDate%>",
      "modifiedLabel": "modified by <%=modifiedBy%> on <%=modifiedDate%>"
    },

this change will be applied the file messages_en.json in OHDSI/WebAPI git.
and consequently the other 3 languages json will be changed like:

messages_ko.json:

    "authorship": {
      "createdLabel": "<%=createdBy%>가 <%=createdDate%> 경에 생성",
      "modifiedLabel": "<%=modifiedBy%>가 <%=modifiedDate%> 경에 수정"
    },

messages_ru.json:

    "authorship": {
      "createdLabel": "создано: <%=createdBy%> <%=createdDate%>",
      "modifiedLabel": "изменено: <%=modifiedBy%> <%=modifiedDate%>"
    },

messages_zh.json:

    "authorship": {
      "createdLabel": "被创造 由 <%=createdBy%>创建 基于/在 <%=createdDate%>",
      "modifiedLabel": "修改的 由 <%=modifiedBy%>修改 基于/在 <%=modifiedDate%>"
    },

lastly, the authorship.html in this Atlas git will be changed like:

<div data-bind="css: classes('container')">
    <span data-bind="visible: createdBy, 
	    text: ko.i18nformat('components.authorship.createdLabel', 
					'by <%=createdBy%> on <%=cratedDate%>', 
					{createdBy: createdBy, createdDate: createdDate})"></span><span data-bind="visible: modifiedDate">,</span>
    <span data-bind="visible: modifiedDate, 
	    text: ko.i18nformat('components.authorship.modifiedLabel', 
					'by <%=modifiedBy%> on <%=modifiedDate%>', 
					{modifiedBy: modifiedBy, modifiedDate: modifiedDate})"></span>
</div>

If the codes above are the same as you think, I will commit the codes again.

chrisknoll · 2024-08-05T13:37:47Z

Yes, that was exactly what I was thinking. Reduces i18n elements and simplifies code.

I think we have to handle the 'version created' case, which is displayed when you are looking at a version of an asset. It should follow the same pattern to say Version created on <%=createdDate%>, but I'd have to look at the code to identify where that i18n element is used to see how they are building the label.

tandara0 · 2024-08-06T03:15:48Z

I tried adding preview element and removing versionCreated in the authorship component.
And in html, I added previewText. Instead, the createdText is no more used.
The previewText will be set in the getAuthorship() javascript function.

messages_en.json:
preview is added and previewText is used. remove versionCreated.

    "authorship": {
      "preview": "version ",
      "createdLabel": "<%=previewText%>created by <%=createdBy%> on <%=createdDate%>",
      "modifiedLabel": "modified by <%=modifiedBy%> on <%=modifiedDate%>"
    },

cohort-definition-manager.js :
previewText is added.

  getAuthorship() {
	const cohortDef = this.currentCohortDefinition();

	let createdText, createdBy, createdDate, modifiedBy, modifiedDate, previewText;

	if (this.previewVersion()) {
		previewText = ko.i18n('components.authorship.preview', 'version ');
                 ......
	} else {
		previewText = null;
                 .......
	}

	return {
		createdText: createdText,
		createdBy: createdBy,
		createdDate: createdDate,
		modifiedBy: modifiedBy,
		modifiedDate: modifiedDate,
		previewText: previewText
	}
  }

Like above code changes of cohort-defined-manager.js, all javascript with getAuthorship() function defined should be changed.

authorship.js:
previewText is added.

	class Authorship extends AutoBind(Component) {
		constructor(params) {
			super(params);
			this.createdText = params.createdText || ko.i18n('components.authorship.created', 'created');
			this.createdBy = params.createdBy;
			this.createdDate = params.createdDate;
			this.modifiedBy = params.modifiedBy;
			this.modifiedDate = params.modifiedDate;
			this.previewText = params.previewText;
		}
	}

authorship.html:
use previewText.

<div data-bind="css: classes('container')">
	<span data-bind="visible: createdBy,
	    text: ko.i18nformat('components.authorship.createdLabel',
						'<%=previewText%>created by <%=createdBy%> on <%=cratedDate%>',
						{createdBy: createdBy, createdDate: createdDate, previewText:previewText})"></span><span data-bind="visible: modifiedDate">,</span>
    <span data-bind="visible: modifiedDate,
	    text: ko.i18nformat('components.authorship.modifiedLabel',
						'modified by <%=modifiedBy%> on <%=modifiedDate%>',
						{modifiedBy: modifiedBy, modifiedDate: modifiedDate})"></span>
</div>

Finally, the createdText is no more used, and can be eliminated.

chrisknoll · 2024-08-06T04:20:02Z

Ok, thanks for explaining this, and seems fine. Only thing I would change is on the naming: the 'preview' is probably better termed a 'prefix' or 'preamble', prefix is probably fine enough (as it represents something inserted before something else). while preamble is more like a introduction to a story. So I'd suggest prefix over preview (preview has different meaning in the application where you can 'preview' a concept set change before commit).

tandara0 · 2024-08-06T07:26:29Z

Although I don't want to insist on my codes, I think I have to add a explanation as there is possible that you misunderstood something.

I will show you the changed json of messages_ko.json

messages_ko.json:

    "authorship": {
      "preview": "버전 ",
      "createdLabel": "<%=createdBy%>가 <%=createdDate%> 경에 <%=previewText%>생성",
      "modifiedLabel": "<%=modifiedBy%>가 <%=modifiedDate%> 경에 수정"
    },

As you can see in this json, the previewText is not positioned in front of the sentense, in korean.
(It is not used as prefix.)
I named preview because the components.authorship.versionCreated is used only when the contents is in previewVersion.
(see getAuthorship() in cohort-definition-manager.js.)

I agree the name 'preview' is not the best.
Other candidates are likely to be 'version', 'previewVersion', 'versionCreated', or else.

So, just think about it one more time and make a decision, then I'll follow any decision.

chrisknoll · 2024-08-06T14:22:03Z

Ok, thanks for the clarification. So I think in order to to deal with different sentence structure across languages, we need to think in terms of labels. I think i understand that you are saying that the 'preview' element positioning depends on langauge. So i think we need to think about making it a complete sentence (for the preview label) so it handles the differences.

I don't use the version history so much that I don't know if it even matters that we have to have special messaging about authorship for a preview vs. the normal, non-version-history view. I would think that when the version history shows up, you have something in the dialog box or in the header that says 'this is a version'. So could we remove the preview text from the authorshp component completely?

Signed-off-by: Juyoung Kim <tandara0@gmail.com>

tandara0 · 2024-08-07T01:28:36Z

Sure.
I remove the preview text.
Then I committed both Atlas and WebAPI.

The WebAPI PR: OHDSI/WebAPI#2382

Please, review them.

fix The Korean sentence order in Authorship description

00c8da5

Signed-off-by: Juyoung Kim <tandara0@gmail.com>

refactoring i18n authorship component then applying to Atlas UI.

71d76af

Signed-off-by: Juyoung Kim <tandara0@gmail.com>

chrisknoll approved these changes Aug 29, 2024

View reviewed changes

chrisknoll merged commit 934ce42 into OHDSI:master Aug 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix The Korean sentence order in Authorship description#2947

fix The Korean sentence order in Authorship description#2947
chrisknoll merged 2 commits intoOHDSI:masterfrom
tandara0:fix-i18n-korean

tandara0 commented Jul 31, 2024

Uh oh!

chrisknoll commented Aug 2, 2024 •

edited

Loading

Uh oh!

tandara0 commented Aug 5, 2024 •

edited

Loading

Uh oh!

chrisknoll commented Aug 5, 2024 •

edited

Loading

Uh oh!

tandara0 commented Aug 6, 2024 •

edited

Loading

Uh oh!

chrisknoll commented Aug 6, 2024

Uh oh!

tandara0 commented Aug 6, 2024

Uh oh!

chrisknoll commented Aug 6, 2024

Uh oh!

tandara0 commented Aug 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

tandara0 commented Jul 31, 2024

Uh oh!

chrisknoll commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tandara0 commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisknoll commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tandara0 commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisknoll commented Aug 6, 2024

Uh oh!

tandara0 commented Aug 6, 2024

Uh oh!

chrisknoll commented Aug 6, 2024

Uh oh!

tandara0 commented Aug 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chrisknoll commented Aug 2, 2024 •

edited

Loading

tandara0 commented Aug 5, 2024 •

edited

Loading

chrisknoll commented Aug 5, 2024 •

edited

Loading

tandara0 commented Aug 6, 2024 •

edited

Loading