Skip to content

New theme-able components#1748

Merged
tdonohue merged 7 commits intoDSpace:mainfrom
mspalti:new-themes
Sep 22, 2022
Merged

New theme-able components#1748
tdonohue merged 7 commits intoDSpace:mainfrom
mspalti:new-themes

Conversation

@mspalti
Copy link
Copy Markdown
Member

@mspalti mspalti commented Aug 4, 2022

Description

This PR converts CommunityPageSubCommunityListComponent and CommunityPageSubCollectionListComponent into theme-able components.

It also introduces a new optional pageSize Input to these two components. The new input allows the user to override the hard-coded default page size of "5" .

For example, this change in the theme's version of community-page.component.html will set the initial page size to 20 results.

<ds-themed-community-page-sub-community-list [community]="communityPayload" [pageSize]="20"></ds-themed-community-page-sub-community-list>
<ds-themed-community-page-sub-collection-list [community]="communityPayload" [pageSize]="20"></ds-themed-community-page-sub-collection-list>

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 TSLint validation using yarn run lint
  • My PR doesn't introduce circular dependencies
  • 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, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

@tdonohue tdonohue added this to the 7.4 milestone Aug 9, 2022
@tdonohue
Copy link
Copy Markdown
Member

tdonohue commented Aug 9, 2022

@artlowel : Assigning to you in case you have a chance to review this quickly (as I'm heading off on one last holiday tomorrow)

Copy link
Copy Markdown
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 @mspalti

It works mostly, there's just a typo in the path for the ThemedCollectionPageSubCollectionListComponent

@mspalti
Copy link
Copy Markdown
Member Author

mspalti commented Sep 21, 2022

This is ready for review.

@tdonohue tdonohue requested a review from artlowel September 21, 2022 15:26
@tdonohue tdonohue self-requested a review September 21, 2022 15:26
Copy link
Copy Markdown
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 @mspalti!

Copy link
Copy Markdown
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 @mspalti , looks good to me too. I gave this a quick double check with the custom theme and it's working.

@tdonohue tdonohue merged commit 001d6dc into DSpace:main Sep 22, 2022
@mspalti
Copy link
Copy Markdown
Member Author

mspalti commented Oct 21, 2022

@tdonohue , @artlowel , this PR was merged into 7.4 but I want to run something by you. It might be a problem.

As noted above, the PR adds an optional pageSize input to the sub-collection, sub-community list components. The original idea was to override the hard-coded "5" item limit in these components. But later I dropped the hard-coded limits entirely, because mistakenly I assumed that new pageSize configuration options had an effect here. I now see that's not the case (and shouldn't be the case)!

So in 7.4, it looks like the sub lists default to 10 items. That's probably an unintended change from 7.3, although I personally prefer it.

IMHO, the pageSize for sub community/collection pagination is something else that we should add to the UI configuration options. So if there's something that needs fixing here, I'd recommend that over reintroducing a hard limit into these list components.

@mspalti
Copy link
Copy Markdown
Member Author

mspalti commented Oct 21, 2022

It's hard to find examples on the demo site. Here's one community with more than 5 collections.

https://demo7.dspace.org/communities/7328bdc7-7ce8-4f0d-8fad-a347a06388e8

It would be great to hear that this isn't new behavior.

@tdonohue
Copy link
Copy Markdown
Member

tdonohue commented Oct 21, 2022

@mspalti : I honestly don't recall what the pagination behavior was prior to this PR...so that's not something I can answer. If the pagination defaults changed slightly, I also don't consider that a major concern until/unless someone complains.

However, if you see a bug here in that the page is still using a hardcoded pageSize, then I'd recommend creating a bug ticket & describing what you see and how to reproduce it. That way we can find a volunteer to help fix that hardcode pageSize.

@mspalti mspalti deleted the new-themes branch October 13, 2023 19:08
4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request May 29, 2024
…Space#1748)

Fix several issue with SSR

Approved-by: Andrea Barbasso
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