Skip to content

Make the size of a browse result page configurable.#1771

Merged
tdonohue merged 8 commits intoDSpace:mainfrom
mwoodiupui:browse-pagesize
Sep 21, 2022
Merged

Make the size of a browse result page configurable.#1771
tdonohue merged 8 commits intoDSpace:mainfrom
mwoodiupui:browse-pagesize

Conversation

@mwoodiupui
Copy link
Copy Markdown
Member

@mwoodiupui mwoodiupui commented Aug 16, 2022

Description

Invent a new configuration property to control the number of results per page when displaying browse results.

Instructions for Reviewers

List of changes in this PR:

  • Create a new configuration property browseBy.pageSize.
  • Define a default value wherever browseBy is being configured.
  • Replace hard-coded "20" with the value of this property in the browse-by-metadata component.

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.

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 @mwoodiupui

The code looks good, and it works as it should. One strange thing though is that you can't use a number that isn't part of the dropdown of page-sizes on the browse page. It will be rounded to the closest match.

I'm not saying you should change that in this PR (e.g. by making those options configurable as well), but it's likely a good idea to make that clear in the comments

@mwoodiupui mwoodiupui requested a review from artlowel September 2, 2022 20:51
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 @mwoodiupui!

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.

Sorry, I know I already approved, but I noticed something wile reviewing the other RPP PR, I missed here:

id: BBM_PAGINATION_ID,
currentPage: 1,
pageSize: 20
pageSize: environment.browseBy.pageSize,
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.

It's better to inject the environment using

@Inject(APP_CONFIG) protected appConfig: AppConfig

in the constructor, and then use this.appConfig.browseBy.pageSize later on. That makes it easier for us to mock the environment in tests.

@mwoodiupui
Copy link
Copy Markdown
Member Author

mwoodiupui commented Sep 15, 2022

@artlowel "Error: 121:3 error Angular will not invoke the ngOnInit lifecycle method within @Injectable() classes @angular-eslint/contextual-lifecycle"

Suggestions?

@artlowel
Copy link
Copy Markdown
Member

@mwoodiupui I haven't seen it before, but I'd bet the issue is the @Injectable() you added at the top. that's not needed. The @component() decorator already ensures you can use injected services in the constructor.

@mwoodiupui mwoodiupui requested a review from artlowel September 19, 2022 17:59
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 @mwoodiupui!

@mwoodiupui mwoodiupui requested a review from tdonohue September 21, 2022 12:23
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 @mwoodiupui !

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 component: Discovery related to discovery search or browse system configuration improvement quick win Pull request is small in size & should be easy to review and/or merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants