Skip to content

Make no. of communities per pagination / expansion configurable#1803

Merged
tdonohue merged 2 commits intoDSpace:mainfrom
mark-cooper:browse-comm-lists-configurable
Sep 20, 2022
Merged

Make no. of communities per pagination / expansion configurable#1803
tdonohue merged 2 commits intoDSpace:mainfrom
mark-cooper:browse-comm-lists-configurable

Conversation

@mark-cooper
Copy link
Copy Markdown
Contributor

References

Description

Adds new config settings for no. of communities to display on the
home page & community-list page.

Instructions for Reviewers

Compare result of updating the config, for example:

browseCommunities:
  communityListPageSize: 25
  topLevelPageSize: 10

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 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 usability configuration 1 APPROVAL pull request only requires a single approval to merge labels Sep 1, 2022
@tdonohue tdonohue added this to the 7.4 milestone Sep 1, 2022
@tdonohue tdonohue requested a review from artlowel September 1, 2022 14:25
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 @mark-cooper!

This works the way it should, I just have a few inline comments on the code. One strange thing I also noticed in the other RPP PR (#1771), is that no matter what you set the topLevelPageSize to, it will always round to the nearest number from the list of page sizes. e.g. if you set it to 7 it'll use 10.

It would be good to add a comment above the param in the example config to let people know about that

@mark-cooper mark-cooper force-pushed the browse-comm-lists-configurable branch 2 times, most recently from db4ad7f to 9006ca6 Compare September 8, 2022 21:08
@mark-cooper
Copy link
Copy Markdown
Contributor Author

@artlowel thx for the feedback. I'm not sure if the convention is to have the PR author or reviewer resolve conversations so I'll leave it for you.

# No. of communities to list per expansion (show more)
pageSize: 20

home:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@mark-cooper : Quick comment...Could we change this to say homePage:? We have a different PR (#1801) which is creating a section called homePage and it'd be good to align these two PRs so that we have a single section for all homepage configs.

It's unclear which PR will be ready to merge first, but both PRs also have a homepage-config.interface.ts sort of file...and we'll need to find a way to merge the two together.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@tdonohue no problem! Also I updated things to be more aligned with #1801 so there's less to do bringing them together.

@mark-cooper mark-cooper force-pushed the browse-comm-lists-configurable branch from 9006ca6 to 36afbd6 Compare September 13, 2022 19:02
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 @mark-cooper!

@tdonohue
Copy link
Copy Markdown
Member

@mark-cooper : I just merged #1801 , which was the PR that overlapped slightly with this one in terms of the homePage configuration section. Could you rebase this PR now on main to resolved the conflicts? It should also simplify this PR as the homepage-config.interface.ts already exists on main now and you'll just need to update it.

Once this is rebased, I'll do a final test & then this will be ready to merge

Adds new config settings for no. of communities to display on the
home page & community-list page.

Resolves DSpace#1749, resolves DSpace#1750
Inject environment rather than importing it
Redo the configuration for better consistency across pages
@mark-cooper mark-cooper force-pushed the browse-comm-lists-configurable branch from 36afbd6 to 086bd47 Compare September 19, 2022 22:14
@mark-cooper
Copy link
Copy Markdown
Contributor Author

@tdonohue updated!

@tdonohue tdonohue self-requested a review September 20, 2022 17:14
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 @mark-cooper ! I gave this one final test after the conflict cleanup, and it's still working perfectly. Code looks good too

@tdonohue tdonohue merged commit 0f1b9bb into DSpace:main Sep 20, 2022
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 configuration usability

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make the number of communities in the Browse by Community page configurable Make the number of communities on the home page configurable

3 participants