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

Make ItemSearchResultListElementComponent themeable. #2828

Closed
wants to merge 2 commits into from

Conversation

mwoodiupui
Copy link
Member

Description

Make ItemSearchResultListElementComponent themeable.

Instructions for Reviewers

List of changes in this PR:

  • Create ThemedItemSearchResultListElementComponent.
  • Add it to SharedModule.
  • Update references of ItemSearchResultListElementComponent

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

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

@mwoodiupui
Copy link
Member Author

WIP because, while this met my immediate need, I discovered a number of additional references to this component which I need to update.

@toniprieto
Copy link
Contributor

@mwoodiupui Does it solve the same issue? #2651

Copy link

github-actions bot commented Mar 8, 2024

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

Copy link

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

@alexandrevryghem
Copy link
Member

I'm closing this PR since it's a duplicate of #2651. We should normally never create themed- versions of dynamic components nor use their selectors in the html files directly. Instead we should use their dynamic component loaders instead. That way it can be themed like the other dynamic components (by proving the correct theme in the decorator as the last parameter)

@mwoodiupui
Copy link
Member Author

I'm happy to see this fixed.

I would have tried to do this the right way if I had known:

  • how to know by looking at it that ItemSearchResultListElementComponent is a "dynamic component"
  • what's a dynamic component?
  • where to find the documentation on how we theme dynamic components

It seems to me that the theming support is still opaque in some areas.

@mwoodiupui mwoodiupui deleted the themed-item-search branch April 15, 2024 12:28
@alexandrevryghem
Copy link
Member

Previously you could easily see that a component was dynamically themeable because they have a custom decorator on top of the component for example @listableObjectComponent (this is the only one that is still used on dspace-8.0-next). Starting from dspace-8.0-next you can still find out if it's a dynamic component if it is defined in one of de the decorator files for example here (info about why this was migrated)

An example on how to theme these can be found here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants