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

feat: use the metadata with correct language #3078

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

pcg-kk
Copy link
Contributor

@pcg-kk pcg-kk commented May 24, 2024

feat: provide a language support for community pages

References

Add references/links to any related issues or PRs. These may include:

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:

  • We provide an form to save multiple languages
  • We changed a way to display texts from specific languages
  • We modify the backend API to save more than one metadata for each type

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!

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes ESLint validation using yarn lint
  • My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@pcg-kk pcg-kk force-pushed the 2056/enabling-translation-of-collection-and-community-names-and-description branch from 3ba78b7 to d4bd7a5 Compare May 27, 2024 18:04
@pcg-kk pcg-kk marked this pull request as ready for review May 27, 2024 18:06
@tdonohue tdonohue added i18n / l10n Internationalisation and localisation, related to message catalogs new feature labels May 28, 2024
@tdonohue tdonohue requested a review from artlowel August 8, 2024 14:37
Copy link

github-actions bot commented Aug 8, 2024

Hi @pcg-kk,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

@pcg-kk pcg-kk force-pushed the 2056/enabling-translation-of-collection-and-community-names-and-description branch from 4610aa1 to 88345f5 Compare August 13, 2024 17:29
Copy link
Member

@artlowel artlowel left a 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)"
Copy link
Member

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

Copy link
Contributor Author

@pcg-kk pcg-kk Feb 10, 2025

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

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.

Copy link
Contributor

@minurmin minurmin Feb 9, 2025

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);
Copy link
Contributor

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)));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx - good remark. Fixed

@pcg-kk pcg-kk force-pushed the 2056/enabling-translation-of-collection-and-community-names-and-description branch from 095747a to 1348c1d Compare February 10, 2025 21:08
@pcg-kk pcg-kk force-pushed the 2056/enabling-translation-of-collection-and-community-names-and-description branch from 1348c1d to cbd44c8 Compare February 10, 2025 21:17
@pcg-kk
Copy link
Contributor Author

pcg-kk commented Feb 10, 2025

@minurmin , @artlowel thanks a lot for review. I updated the repository to be sync with current main and applied your suggestions.

@pcg-kk pcg-kk force-pushed the 2056/enabling-translation-of-collection-and-community-names-and-description branch from 06d67ab to 9518554 Compare February 10, 2025 21:31
} else {
formMetadata[fieldModel.name] = [value];
}
const operations: Map<string, ReplaceOperation<{value: string, language: string}[]>> = new Map();
Copy link
Contributor

@minurmin minurmin Feb 19, 2025

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n / l10n Internationalisation and localisation, related to message catalogs new feature
Projects
Status: 👀 Under Review
Development

Successfully merging this pull request may close these issues.

Enabling translation of collection's/community's names and description
4 participants