-
Notifications
You must be signed in to change notification settings - Fork 447
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
feat: use the metadata with correct language #3078
base: main
Are you sure you want to change the base?
feat: use the metadata with correct language #3078
Conversation
3ba78b7
to
d4bd7a5
Compare
Hi @pcg-kk, |
4610aa1
to
88345f5
Compare
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.
Thanks @pcg-kk!
This works well. I like the decision of putting the translation form next to the original, that makes it very easy to use!
This PR does change the behavior for languages that aren't (entirely) translated though. Currently it would fall back to the default language for fields that don't have a value in the current language. With this PR, fields that don't have a value in the current language are omitted. It would be good to try and retain the original behavior.
I also have a few minor inline comments.
@@ -23,12 +23,12 @@ | |||
</ds-comcol-page-handle> | |||
<!-- Introductory text --> | |||
<ds-comcol-page-content | |||
[content]="collection.introductoryText" | |||
[content]="collection.introductoryText(translateService.currentLang)" |
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'd create a local property in the component to store translateService.currentLang
, and initialize it in ngOnInit
that will likely be more efficient than accessing the service from the HTML. This goes for all the components in this PR that use translateService.currentLang
from the HTML
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.
Fixed
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.
Thanks @pcg-kk, but I see it's still used in a bunch of other components. I'd do a search in your IDE for "translateservice.currentlang" in HTML files, and move it to the ts file for all of them.
src/app/shared/comcol/comcol-forms/comcol-form/comcol-form.component.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.
The line
[content]="collection.copyrightText" |
should include (translateService.currentLang)
(as in community-page.component.html, https://github.com/DSpace/dspace-angular/pull/3078/files#diff-df2430517ff5adc042caccd6792890e6070e1f71221fee6c4fc42c5acfce299dR35 )
Otherwise a text copyrightText(S){return this.firstMetadataValue("dc.rights",{language:S})} would appear at the end of collection page.
this.defaultLanguageCode = environment.defaultLanguage; | ||
this.defaultLanguage = environment.languages.find(lang => lang.code === this.defaultLanguageCode).label; | ||
|
||
this.languages = environment.languages.filter(lang => lang.code !== this.defaultLanguageCode); |
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.
Consider adding only active languages to limit the length of the list. E.g.
this.languages = environment.languages.filter(lang => ((lang.code !== this.defaultLanguageCode) && (lang.active === true)));
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.
Thx - good remark. Fixed
095747a
to
1348c1d
Compare
feat: provide a language support for community pages
feat: first test fix feat: fix unit
1348c1d
to
cbd44c8
Compare
06d67ab
to
9518554
Compare
} else { | ||
formMetadata[fieldModel.name] = [value]; | ||
} | ||
const operations: Map<string, ReplaceOperation<{value: string, language: string}[]>> = new Map(); |
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 function (and more specifically, the blocks starting here and ending to submitForm.emit) is the only place in this PR that depends on DSpace/DSpace#9610. Based on discussion in server-side PR, you might want to consider implementing this based on PATCH ADD so REST Contract would not have to be changed. For example, something like this (additional checks are probably needed and there is room for optimization as I have limited experience with TypeScript):
const operations: Map<string, AddOperation<{value: string, language: string}>[]> = new Map();
(slightly different form of array because Operations are now iterated instead of values)
if ( keyExistAndAtLeastOneValueIsNotNull) {
if (operations.has(key)) {
const fieldOperations: Operation[] = operations.get(key);
fieldOperations.push({
op: 'add',
path: key,
value: {
value: fieldModel.value as string,
language
}
});
} else {
operations.set(key, [
{op: 'add',
path: key,
value: {
value: fieldModel.value as string,
language
},
}]);
}
}
(construct an array of add operations instead of array of values)
let finalOperations: Operation[] = [];
for (let [key,value] of operations) {
finalOperations.push({op: 'remove', path: key}),
finalOperations = finalOperations.concat(value);
};
(before submitForm.emit construct another array that is suitable as such for the emit method instead of operations.values() and includes remove operation to ensure that old fields are cleared)
feat: provide a language support for community pages
References
Add references/links to any related issues or PRs. These may include:
replace
PATCH request on /metadata now supports multiple values DSpace#9610 (if a REST API PR is required to test this)Description
In this PR we provide an mechanism to upload more than one language for communities and collections pages. User can define a page in all supported by DSpace languages from an customer environment file.
Instructions for Reviewers
Please add a more detailed description of the changes made by your PR. At a minimum, providing a bulleted list of changes in your PR is helpful to reviewers.
List of changes in this PR:
Include guidance for how to test or review your PR. This may include: steps to reproduce a bug, screenshots or description of a new feature, or reasons behind specific changes.
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
yarn lint
yarn check-circ-deps
)package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.