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

Copy SearchContextComponent to ServerItemPaginationComponent to ease BL8 upgrade path #3421

Merged

Conversation

maxkadel
Copy link
Contributor

  • Copy Blacklight::SearchContextComponent to Blacklight::SearchContext::ServerItemPaginationComponent to make it easier to upgrade from Blacklight 7 to Blacklight 8
  • Update signature for ServerItemPaginationComponent to include current_document to ease upgrade path (not yet used in the class)
  • Add deprecation warning for Blacklight::SearchContextComponent

Closes #3313

end

def render?
@search_context.present? && (@search_context[:prev] || @search_context[:next])
Copy link
Member

Choose a reason for hiding this comment

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

Blacklight 8 has this as:

Suggested change
@search_context.present? && (@search_context[:prev] || @search_context[:next])
@search_context.present? && (@search_context[:prev] || @search_context[:next] || total.positive?) && (@search_session['document_id'] == @current_document_id)

Should this match, or is there a reason this is different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that version and it wasn't compatible with Blacklight 7.

My hope with this PR is that we will be able to add a component to replace our custom pagination partial that will work with both Blacklight 7 and Blacklight 8 without significant revisions when upgrading between versions.

@jrochkind
Copy link
Member

I think I follow what's going on, but can you briefly explain how this eases BL8 upgrade path?

Also, there are no tests here -- Can you explain your attitude toward tests here? Perhaps we don't need tests, because existing tests cover all necessary things to test, that existing tests continue to be green under this refactor is sufficient? I'm not sure.

@maxkadel
Copy link
Contributor Author

I think I follow what's going on, but can you briefly explain how this eases BL8 upgrade path?

Also, there are no tests here -- Can you explain your attitude toward tests here? Perhaps we don't need tests, because existing tests cover all necessary things to test, that existing tests continue to be green under this refactor is sufficient? I'm not sure.

My sense was that the existing tests were sufficient for the refactor, but I can try to write some new ones or pull from main for the components.

@jrochkind
Copy link
Member

Oh don't add new tests on my account, I was just confirming it was intentional not to include any,that's fine with me!

- Test components

Co-authored-by: Jane Sandberg <[email protected]>
@maxkadel
Copy link
Contributor Author

I think I follow what's going on, but can you briefly explain how this eases BL8 upgrade path?

Sorry, realized I didn't explicitly respond to this part @jrochkind.

We have a custom partial for our _previous_next_doc. To upgrade we need to migrate this to a view component. For it to work on Blacklight 7 right now, that view component needs to be a SearchContextComponent (or inherit from it), but for it to work in Blacklight 8 it needs to be a ServerItemPaginationComponent, which does not exist in Blacklight 7. We're trying to find a way to have code that will work in future with Blacklight 8, but that will continue to work with Blacklight 7 in the meantime.

Also, having accurate deprecation statements is helpful! But it's hard when they say to use a thing you cannot actually use!

Copy link
Contributor

@sandbergja sandbergja left a comment

Choose a reason for hiding this comment

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

Thanks, @maxkadel !


module Blacklight
module SearchContext
class ServerItemPaginationComponent < Blacklight::SearchContextComponent
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this approach of inheriting from the Blacklight 7 component, and providing an interface that is compatible with the Blacklight 8 component!

@@ -106,7 +106,7 @@ def item_page_entry_info
total: number_with_delimiter(search_session['total']),
count: search_session['total'].to_i).html_safe
end
deprecation_deprecate item_page_entry_info: 'Use Blacklight::SearchContextComponent methods instead'
deprecation_deprecate item_page_entry_info: 'Use Blacklight::SearchContext::ServerItemPaginationComponent methods instead'
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

@sandbergja sandbergja merged commit 9409ccc into projectblacklight:release-7.x Nov 5, 2024
11 checks passed
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.

4 participants