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

Fixed ItemSearchResultListElementComponent not fully themable #2651

Merged
merged 10 commits into from
May 3, 2024

Conversation

alexandrevryghem
Copy link
Member

References

Description

The ItemListElementComponent previously used the ItemSearchResultListElementComponent selector directly in its HTML, instead of using the ListableObjectComponentLoaderComponent. Because of this the base theme is always used instead of the correct one.

Instructions for Reviewers

List of changes in this PR:

  • Updated ItemListElementComponent to use the ListableObjectComponentLoaderComponent.
  • Created ItemListElementComponent#transformItemToItemSearchResult to convert an Item to an ItemSearchResult.

Guidance for how to test or review this PR:

  • Theme the ItemSearchResultListElementComponent in your theme (or use the custom theme, but don't forget to uncomment the theme in src/themes/eager-themes.module.ts)
  • Verify that with this PR the ItemSearchResultListElementComponent in the browse by pages is now also correctly themed

Checklist

  • 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.

…ResultListElementComponent from the themes folder when present
@alexandrevryghem alexandrevryghem self-assigned this Nov 18, 2023
@alexandrevryghem alexandrevryghem added bug themes claimed: Atmire Atmire team is working on this issue & will contribute back labels Nov 18, 2023
@tdonohue tdonohue added port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release 1 APPROVAL pull request only requires a single approval to merge labels Nov 21, 2023
Copy link

github-actions bot commented Feb 9, 2024

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

…ute-main

# Conflicts:
#	src/app/shared/mydspace-actions/claimed-task/switcher/claimed-task-actions-loader.component.ts
#	src/app/shared/object-collection/shared/listable-object/listable-object-component-loader.component.spec.ts
Copy link

github-actions bot commented Mar 8, 2024

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

…ute-main

# Conflicts:
#	src/app/shared/object-collection/shared/listable-object/listable-object-component-loader.component.spec.ts
#	src/app/shared/object-list/item-list-element/item-types/item/item-list-element.component.spec.ts
#	src/app/shared/object-list/item-list-element/item-types/item/item-list-element.component.ts
Copy link

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

Copy link

github-actions bot commented Apr 3, 2024

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

…ute-main

# Conflicts:
#	src/app/shared/object-collection/shared/listable-object/listable-object-component-loader.component.spec.ts
#	src/app/shared/object-list/item-list-element/item-types/item/item-list-element.component.spec.ts
Copy link

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

…ute-main

# Conflicts:
#	src/app/shared/object-collection/shared/listable-object/listable-object-component-loader.component.spec.ts
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@alexandrevryghem : For some reason, with this PR installed, SSR hangs indefinitely. It's possible this has conflicts with recent changes like #2865

To reproduce:

  1. Build this PR in production mode by pulling this code on top of latest main and running: yarn install; yarn build:prod
  2. Run via yarn serve:ssr
  3. Attempt to access the homepage. The page will never load.

The e2e tests that are run by this PR show a similar behavior.

@alexandrevryghem
Copy link
Member Author

@tdonohue: I merged the latest main branch and the error was automatically fixed, so this was probably because of a recent change on main

@tdonohue
Copy link
Member

tdonohue commented May 1, 2024

@alexandrevryghem : This appears to be failing specs at this time. Once that's resolved, I can retest. Here's the error:

FAILED TESTS:
  ItemListElementComponent
    when the publication is rendered
      ✖ should contain a PublicationListElementComponent
        Chrome Headless 124.0.6367.60 (Linux x86_64)
      TypeError: Cannot read properties of undefined (reading 'getRenderTypes')
          at ListableObjectComponentLoaderComponent.getComponent (src/app/shared/object-collection/shared/listable-object/listable-object-component-loader.component.ts:131:51)
          at ListableObjectComponentLoaderComponent.instantiateComponent (src/app/shared/abstract-component-loader/abstract-component-loader.component.ts:112:51)
          at ListableObjectComponentLoaderComponent.instantiateComponent (src/app/shared/object-collection/shared/listable-object/listable-object-component-loader.component.ts:114:11)
          at ListableObjectComponentLoaderComponent.call [as ngOnInit] (src/app/shared/abstract-component-loader/abstract-component-loader.component.ts:83:10)
          at callHookInternal (node_modules/@angular/core/fesm2022/core.mjs:5136:14)
          at callHook (node_modules/@angular/core/fesm2022/core.mjs:5163:13)
          at callHooks (node_modules/@angular/core/fesm2022/core.mjs:5118:17)
          at executeInitAndCheckHooks (node_modules/@angular/core/fesm2022/core.mjs:5068:9)
          at refreshView (node_modules/@angular/core/fesm2022/core.mjs:12806:21)
          at detectChangesInView$1 (node_modules/@angular/core/fesm2022/core.mjs:13015:9)

@alexandrevryghem alexandrevryghem force-pushed the theme-fixes_contribute-main branch from 2c6d04f to da320b5 Compare May 1, 2024 17:10
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @alexandrevryghem ! This now appears to be working properly. I tested it via the custom theme and dspace theme, and both appear to be working.

@tdonohue tdonohue merged commit a65f248 into DSpace:main May 3, 2024
13 checks passed
@dspace-bot
Copy link
Contributor

Backport failed for dspace-7_x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin dspace-7_x
git worktree add -d .worktree/backport-2651-to-dspace-7_x origin/dspace-7_x
cd .worktree/backport-2651-to-dspace-7_x
git switch --create backport-2651-to-dspace-7_x
git cherry-pick -x d00fc5d9d300ae85cdb3ae82271650d04d48d774 04cf1419eaaa440e94927687bc54154f0dcd6aa5 da320b5fbc52fdc2facc67a55cbd129ed423545e

@tdonohue
Copy link
Member

tdonohue commented May 3, 2024

It looks like the port to 7.x failed. If we want to port this to 7.x, that'd need to be in a separate PR. That said, I'm OK with this also just being an 8.0 change.

@tdonohue tdonohue added this to the 8.0 milestone May 3, 2024
@alexandrevryghem alexandrevryghem deleted the theme-fixes_contribute-main branch May 9, 2024 10:21
@alexandrevryghem alexandrevryghem removed the port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release label May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge bug claimed: Atmire Atmire team is working on this issue & will contribute back themes
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

ItemSearchResultListElementComponent not fully themable
3 participants