Add lang attribute to elements showing metadata values#4776
Conversation
|
@tdonohue, tests are failing because of invalid values for the A lot of metadata fields have the language set to EDIT: after further research, I see that the difference is caused by different standards for in web technologies (IETF BCP 47) and Java/POSIX systems. |
|
As discussed today, I am going to open an issue on the REST side to discuss if we need to change the chosen standard for language codes. |
|
Hi @AndreaBarbasso, |
# Conflicts: # src/app/entity-groups/journal-entities/item-list-elements/search-result-list-elements/journal-issue/journal-issue-search-result-list-element.component.ts # src/app/entity-groups/journal-entities/item-list-elements/search-result-list-elements/journal-volume/journal-volume-search-result-list-element.component.ts # src/app/entity-groups/journal-entities/item-list-elements/search-result-list-elements/journal/journal-search-result-list-element.component.ts # src/app/entity-groups/research-entities/item-list-elements/search-result-list-elements/org-unit/org-unit-search-result-list-element.component.ts # src/app/entity-groups/research-entities/item-list-elements/search-result-list-elements/person/person-search-result-list-element.component.ts # src/app/entity-groups/research-entities/submission/item-list-elements/org-unit/org-unit-search-result-list-submission-element.component.ts # src/app/entity-groups/research-entities/submission/item-list-elements/person/person-search-result-list-submission-element.component.ts # src/app/shared/object-detail/my-dspace-result-detail-element/item-detail-preview/item-detail-preview-field/item-detail-preview-field.component.ts # src/app/shared/object-detail/my-dspace-result-detail-element/search-result-detail-element.component.ts # src/app/shared/object-grid/search-result-grid-element/search-result-grid-element.component.ts # src/app/shared/object-list/metadata-representation-list-element/browse-link/browse-link-metadata-list-element.component.ts # src/app/shared/object-list/metadata-representation-list-element/plain-text/plain-text-metadata-list-element.component.ts # src/app/shared/object-list/search-result-list-element/collection-search-result/collection-search-result-list-element.component.ts # src/app/shared/object-list/search-result-list-element/community-search-result/community-search-result-list-element.component.ts # src/app/shared/object-list/search-result-list-element/search-result-list-element.component.ts # src/themes/custom/app/shared/object-list/search-result-list-element/item-search-result/item-types/item/item-search-result-list-element.component.ts
|
Hi @AndreaBarbasso, |
# Conflicts: # src/app/entity-groups/research-entities/metadata-representations/org-unit/org-unit-item-metadata-list-element.component.html # src/app/entity-groups/research-entities/metadata-representations/org-unit/org-unit-item-metadata-list-element.component.ts # src/app/entity-groups/research-entities/metadata-representations/person/person-item-metadata-list-element.component.html # src/app/entity-groups/research-entities/metadata-representations/person/person-item-metadata-list-element.component.ts # src/app/shared/object-list/my-dspace-result-list-element/item-list-preview/item-list-preview.component.html # src/app/shared/object-list/search-result-list-element/item-search-result/item-types/item/item-search-result-list-element.component.html
|
QA testing performed (no code review).
|
|
Hi @AndreaBarbasso, |
|
@AndreaBarbasso : Could you update this PR to solve the merge conflict when you have a chance? Thanks! |
…T-19328 # Conflicts: # src/app/item-page/field-components/metadata-values/metadata-values.component.html # src/app/item-page/field-components/metadata-values/metadata-values.component.spec.ts # src/app/shared/object-list/my-dspace-result-list-element/item-list-preview/item-list-preview.component.html # src/app/shared/object-list/my-dspace-result-list-element/item-list-preview/item-list-preview.component.ts # src/app/shared/object-list/search-result-list-element/item-search-result/item-types/item/item-search-result-list-element.component.html # src/app/shared/object-list/search-result-list-element/item-search-result/item-types/item/item-search-result-list-element.component.ts # src/themes/custom/app/shared/object-list/my-dspace-result-list-element/item-list-preview/item-list-preview.component.ts # src/themes/custom/app/shared/object-list/search-result-list-element/item-search-result/item-types/item/item-search-result-list-element.component.ts
tdonohue
left a comment
There was a problem hiding this comment.
@AndreaBarbasso : Thanks for this PR. I've tested & reviewed it today. Overall, the basics are working...but I can reproduce the issues reported by others above:
- In the search results, the
langattribute never exists if hit highlighting has occurred. To reproduce:- Find an Item, add an abstract and set the language.
- Verify the
langattribute exists in the Abstract on the Item page - Now search for a term in the abstract.
- Verify the
langattribute does NOT exist in the Abstract in the search results when hit highlighting occurs. Is it supposed to have alangattribute? - (However, I can verify the
langattribute does exist if you just browse to the item and don't trigger hit highlighting)
- The title of an Item never shows a
langattribute. To reproduce:- Find an Item and set the language of the Title (
dc.title) to "en" (or any other value). - Verify on the Item page that the Title field doesn't have a
langattribute`. - This seems like a bug as the Title should display a language when one exists.
- Find an Item and set the language of the Title (
I have a few other inline comments below. There's a few methods that are repeated in several areas of the code and I think it'd be better to not repeat them. I'm also trying to understand why you needed to create these methods, so I feel they need a better description.
Beyond those issues, the basic functionality is working well.
| * @param {string|string[]} keyOrKeys The metadata key(s) in scope. Wildcards are supported; see [[Metadata]]. | ||
| * @returns {MetadataValue[]} the matching values or an empty array. | ||
| */ | ||
| allMetadata(keyOrKeys: string | string[]): MetadataValue[] { |
There was a problem hiding this comment.
Could you explain why this change is necessary in order to add the lang attribute? Why are we having to change our approach to getting all the metadata to include also the hitHighlights? I'm just not understanding the purpose behind this change, so that implies we may need to describe this method better as to why we need to do this.
There was a problem hiding this comment.
@tdonohue, to create this method I simply took the previously used method and changed it so that it returns an array of Metadata objects instead of an array of strings. I did this to avoid any regression issue and keep backwards compatibility.
I agree with it being found in too many places - that's an ongoing issue with some other methods in these files, but I can start by applying some DRY on this method!
| * @param {string|string[]} keyOrKeys The metadata key(s) in scope. Wildcards are supported; see [[Metadata]]. | ||
| * @returns {MetadataValue[]} the matching values or an empty array. | ||
| */ | ||
| allMetadata(keyOrKeys: string | string[]): MetadataValue[] { |
There was a problem hiding this comment.
Noticing this same method is repeated in several components. Is there a way we could pull this out into a utility method instead of repeating it over and over again?
| * @param {string|string[]} keyOrKeys The metadata key(s) in scope. Wildcards are supported; see [[Metadata]]. | ||
| * @returns {MetadataValue} the first matching value, or `undefined`. | ||
| */ | ||
| firstMetadata(keyOrKeys: string | string[]): MetadataValue { |
There was a problem hiding this comment.
Same here. This is repeated in several components. It'd be better not to repeat the same methods over and over...because if we find a bug in one we may not remember to fix all of them.
|
Hi @AndreaBarbasso, |
…19328 # Conflicts: # src/app/item-page/field-components/metadata-values/metadata-values.component.html
|
@tdonohue, I applied the same logic to dso titles and |
94ae8fa to
5a7f8f8
Compare
tdonohue
left a comment
There was a problem hiding this comment.
@AndreaBarbasso : I retested this today. While the hit highlighting bug is fixed, I'm still not seeing the lang=[lang-code] attribute on Item titles on the Item page. Here's how to reproduce it:
- Administer an Item and set the language of the Title (dc.title) to "en" (or any other value).
- Go back to the Item's page and view the source of the page.
- Notice that the Title field (displayed in an
<h1>) does NOT have alangattribute. - However, if you search for that Item by it's title, the
langattribute WILL APPEAR if you view the source of the search results page.
|
@tdonohue, my bad, I was so focused on the item title in search result elements that I forgot that part. |
tdonohue
left a comment
There was a problem hiding this comment.
👍 Thanks @AndreaBarbasso ! I've verified this is now working properly. Looks good to me now.


References
Description
Applied
langattribute to HTML tags containing metadata with an explicitlanguagevalue.Instructions for Reviewers
The
langattribute has been added via different approaches:innerHTML, that property has now been replaced with the newdsMetadatadirective, that accepts aMetadataValueand applies theinnerHTMLproperty and thelangattribute to its host element. Having a directive for this purpose is useful since we can later add other attributes if necessary and is more feasible than a component.langattribute has been applied to the closest available element.List of changes in this PR:
dsMetadatadirective;langattribute where using the directive could lead to unwanted behaviors.To test the PR, change any metadata
languagevalue and make sure that the value is added to thelangattribute of its containing element, wherever it's reflected (e.g. item pages, search results, ...).Checklist
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.