-
Notifications
You must be signed in to change notification settings - Fork 19
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
Internationalize the UI and add a German localization #756
Conversation
6a6381f
to
8de755b
Compare
Hello and thank you for the contribution! 🚀 We will let you know as soon as we know when this cool feature will be on our product roadmap (@AbdullahMuk). |
Cool PR! We should merge this as soon as possible. The longer we wait the more merge conflicts will be present. @AbdullahMuk |
Thank you for your contribution. I apologize for the delayed merge. We've been on a very tight schedule to deliver the upcoming release, thus we could not afford to incorporate this PR into the code base yet. For the upcoming release (tomorrow) I also do not foresee that we'll have the time to include this. I recommend we do it after that release. |
Thank you for the update, @richardtreier. With @AbdullahMuk, it has been discussed that just before you start the review, we will rebase this branch onto the latest |
I force pushed a rebased version of this feature, resolving the conflicts by accepting all changes made by you in a first step. This is not ready for review yet, because we need to re-add some translations that have to be integrated differently now and add some new translations. |
I'll go ahead and migrate this code to work with our latest changes. |
Hello, I am currently in the progress of refreshing this PR and getting ready to merge. I see some issues with trying to use the "classic" Apache 2.0 license header comments in the files touched by the PR. I would like to switch to a My suggestion would be:
Is that okay with you? |
0911fa1
to
66426e1
Compare
# Conflicts: # src/app/app.module.ts # src/app/component-library/catalog/asset-detail-dialog/asset-detail-dialog.component.html # src/app/component-library/catalog/asset-detail-dialog/asset-property-grid-group-builder.ts # src/app/component-library/catalog/catalog.module.ts # src/app/component-library/edit-asset-form/edit-asset-form/edit-asset-form.component.html # src/app/component-library/edit-asset-form/language-select/language-select.component.html # src/app/component-library/json-dialog/json-dialog/json-dialog.component.html # src/app/component-library/url-list-dialog/url-list-dialog/url-list-dialog.component.html # src/app/routes/connector-ui/asset-page/asset-edit-dialog/asset-edit-dialog.component.html # src/app/routes/connector-ui/catalog-browser-page/catalog-browser-page/catalog-browser-page.component.ts # src/app/routes/connector-ui/contract-definition-page/contract-definition-cards/contract-definition-cards.component.html # src/app/routes/connector-ui/contract-definition-page/contract-definition-cards/contract-definition-cards.component.ts # src/app/routes/connector-ui/contract-definition-page/contract-definition-editor-dialog/contract-definition-editor-dialog.component.html # src/app/routes/connector-ui/contract-definition-page/contract-definition-page/contract-definition-page.component.html # src/app/routes/connector-ui/dashboard-page/dashboard-page/dashboard-page.component.html
…translations are currently broken TODO
@@ -1,12 +1,14 @@ | |||
<h1 mat-dialog-title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requires detailed review / checking of the individual string values compared to previous version
@@ -1,13 +1,17 @@ | |||
<h1 mat-dialog-title>Initiate Transfer</h1> | |||
<h1 mat-dialog-title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requires detailed review / checking of the individual string values compared to previous version
...ui/contract-definition-page/contract-definition-cards/contract-definition-cards.component.ts
Outdated
Show resolved
Hide resolved
src/app/routes/connector-ui/dashboard-page/dashboard-page/dashboard-page-data.service.ts
Outdated
Show resolved
Hide resolved
...this.propertyGridUtils.guessValue(asset.licenseUrl), | ||
}, | ||
{ | ||
icon: 'category', | ||
label: this.participantIdLocalization.participantId, | ||
label: this.translateService.instant( | ||
'component_library.participant_id', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove component_library
prefix
...this.propertyGridUtils.guessValue(asset.participantId), | ||
}, | ||
{ | ||
icon: 'account_circle', | ||
label: 'Organization', | ||
label: this.translateService.instant('component_library.organization'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove component_library
prefix
@@ -101,15 +117,15 @@ export class AssetPropertyGridGroupBuilder { | |||
|
|||
fields.push({ | |||
icon: 'api', | |||
label: 'HTTP Data Source Parameterization', | |||
label: this.translateService.instant('component_library.http_param'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove component_library
prefix, maybe move to "data_source" or "data_address" as parent term for both data source and data sink
src/app/shared/business/asset-detail-dialog/asset-property-grid-group-builder.ts
Outdated
Show resolved
Hide resolved
src/app/shared/business/asset-detail-dialog/asset-property-grid-group-builder.ts
Outdated
Show resolved
Hide resolved
src/app/shared/business/asset-detail-dialog/asset-property-grid-group-builder.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self-review
src/app/shared/business/asset-detail-dialog/asset-property-grid-group-builder.ts
Outdated
Show resolved
Hide resolved
src/app/shared/business/asset-detail-dialog/asset-property-grid-group-builder.ts
Outdated
Show resolved
Hide resolved
src/app/shared/business/asset-detail-dialog/asset-property-grid-group-builder.ts
Outdated
Show resolved
Hide resolved
src/app/shared/business/asset-detail-dialog/asset-property-grid-group-builder.ts
Outdated
Show resolved
Hide resolved
src/app/shared/business/asset-detail-dialog/asset-property-grid-group-builder.ts
Outdated
Show resolved
Hide resolved
</button> | ||
</mat-chip> | ||
<input | ||
id="create-asset-form-keywords" | ||
placeholder="Add keyword..." | ||
[placeholder]="'asset_list_page.add_keyword' | translate" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key asset_list_page
should be split into asset_list_page
and data_offer_form
or asset_metadata
src/app/shared/pipes-and-directives/pipes-and-directives.module.ts
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,294 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obviously out-of-date, will be handled in #829
@@ -0,0 +1,327 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
partially out-of-date with recent UI wording changes, potentially outdated unused keys, will be fixed in #828
# Conflicts: # src/app/routes/connector-ui/policy-definition-create-page/policy-definition-create-page/policy-definition-create-page-form.ts
PR comments will be left open to be referenced in follow-up issues |
# Conflicts: # src/app/component-library/policy-editor/model/policy-mapper.ts # src/app/shared/business/policy-editor/model/policy-form-adapter.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly just keys that are not descriptive enough which would make further development in the UI annoying because you would need to check the content of every placeholder to be sure what it is
sovity EDC UI | ||
Copyright (c) 2024. sovity GmbH | ||
|
||
This product includes software developed at sovity GmbH (https://www.sovity.de). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disregard if this is copied from somewhere but I would say "by sovity" instead "at sovity" to be more consistent with the wording below "localization to German was done by the Fraunhofer Institute..."
) {} | ||
|
||
private onShowConnectorVersionClick(title: string, versionData: any) { | ||
const data: JsonDialogData = { | ||
title, | ||
subtitle: 'Show Details', | ||
subtitle: this.translateService.instant('general.details'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't general.show_details
be more descriptive?
@@ -86,13 +88,14 @@ export class ConnectorInfoPropertyGridGroupBuilder { | |||
label: 'UI Version', | |||
text: data.trim().toString() | |||
? this.asDate(data.trim().toString()) | |||
: 'Show Details', | |||
: this.translateService.instant('general.details'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't general.show_details
be more descriptive?
@@ -101,7 +104,7 @@ export class ConnectorInfoPropertyGridGroupBuilder { | |||
{ | |||
icon: 'link', | |||
label: 'UI Version', | |||
text: 'Show Details', | |||
text: this.translateService.instant('general.details'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't general.show_details
be more descriptive?
label: this.translateService.instant('general.loading1'), | ||
text: this.translateService.instant('general.loading'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be differentiated more, it will be a pain to reuse this
</p> | ||
</div> | ||
|
||
<div class="w-full flex flex-col justify-center"> | ||
<mat-checkbox [checked]="checkboxChecked" (change)="onCheckboxChange($event)"> | ||
I agree to the Data Offer Terms & Conditions | ||
{{ 'component_library.agree' | translate }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tos_agree
or something?
dialogData.confirmText = 'Delete'; | ||
dialogData.cancelText = 'Cancel'; | ||
dialogData.confirmText = translateService.instant('general.delete'); | ||
dialogData.cancelText = translateService.instant('general.close'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a deliberate choice to go from "Cancel" to "Close"? If not, it's the wrong key. It should be general.cancel
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's an abstract confirmation dialog, that could be "cancel" or "close". In case of a deletion you want it to be "[red] Delete | Close"
(ngModelChange)="updateVisibleJson()" | ||
>Cleaned JSON | ||
(ngModelChange)="updateVisibleJson()"> | ||
{{ 'component_library.json' | translate }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not cleaned.json
? Seeing json
I would expect the value to be "JSON" intuitively.
*ngIf="selectedLanguage.value" | ||
class="h-[1em]" | ||
src="../../../../assets/images/flags/{{ selectedLanguage.value }}.svg" | ||
alt="Selected Flag" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the alt text also be translated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's actually just remove the alt.
@@ -9,7 +9,7 @@ import {LanguageSelectItemService} from './language-select-item.service'; | |||
}) | |||
export class LanguageSelectComponent { | |||
@Input() | |||
label: string = 'Language'; | |||
label: string | null = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're a little inconsistent here. I saw another instance where the string has been initialized as an empty string instead of null.
We should probably decide on one
# Conflicts: # src/app/routes/connector-ui/asset-edit-page/asset-edit-page/asset-edit-page.component.ts
Thank you for your contribution. With the sprawl between the main and this all-files-touching PR some gaps were refined to be fixed in increments on the main. Take care that the current version on the main is thus not production-ready but will be amended before the next release. Synced with @kamilczaja |
Please leave the currently open discussions on this PR open as we'll take care of them in the follow-up |
As discussed with @SebastianOpriel and @richardtreier, we as Fraunhofer FIT provide an internationalization of the UI alongside a German translation as a PR, as part of the project Datenraum Kultur.
The flag icons used for the English and German localization are taken from here and here, respectively. For both images, the licensing information states that they are in the public domain.
Looking forward to your feedback!
Checklist
State of the PR
Written by @richardtreier
The conflicts with parallel changes could not be taken care of completely in this PR: