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

increase the number of AdminSets rendered on #index #1216

Merged
merged 1 commit into from
Jun 16, 2017

Conversation

revgum
Copy link
Contributor

@revgum revgum commented Jun 14, 2017

by default, when visiting the AdminSet #index view, the number of rows rendered is limited to 10. The select list for adjusting viewable rows in the table is not intending on making a request of the server for more/less rows, it just adjusts the visible rows and pagination on the page.

This patch increased the default number of rows to be rendered so that the table is capable of rendering and paginating up to 100 AdminSets.

I don't think this is the best long term solution for this issue, however it does somewhat fall in line with how this is done in the CollectionSearchBuilder. https://github.com/samvera/hyrax/blob/master/app/search_builders/hyrax/collection_search_builder.rb

@@ -8,6 +8,7 @@ class AdminSetSearchBuilder < ::SearchBuilder
# @param [Symbol] access one of :edit, :read, or :deposit
def initialize(context, access)
@access = access
@rows = 100
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to scope this change to AdminSetService#search_results_with_work_count ?

Copy link
Contributor Author

@revgum revgum Jun 14, 2017

Choose a reason for hiding this comment

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

In the case I was addressing (/admin/admin_sets?locale=en) #search_results_with_work_count isn't involved. The request comes in by way of: https://github.com/samvera/hyrax/blob/master/app/controllers/hyrax/admin/admin_sets_controller.rb#L36

Copy link
Member

Choose a reason for hiding this comment

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

Okay, AdminSetService#search_results then?

Copy link
Contributor Author

@revgum revgum Jun 14, 2017

Choose a reason for hiding this comment

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

I had thought about that as well but went with this approach to be consistent with the CollectionSearchBuilder.. So are you thinking,

response = context.repository.search(builder(access))
:

response = context.repository.search(builder(access, 100))

and then setting @rows as an argument in AdminSetSearchBuilder.initialize?

Copy link
Member

Choose a reason for hiding this comment

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

I do not think we should set @rows at all if we can avoid it. We can do AdminSetSearchBuilder.new.rows(100) instead. That's how you're supposed to use that API.

Copy link
Member

Choose a reason for hiding this comment

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

@jcoyne jcoyne merged commit 150e5bc into master Jun 16, 2017
@jeremyf jeremyf deleted the adminset-index-patch branch June 16, 2017 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants