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

Bl 1829 update digital collections results on bento page #4230

Merged

Conversation

cdoyle-temple
Copy link
Contributor

This branch reuses the existing bento result item fields for the CDM field mappings.

@cdoyle-temple cdoyle-temple changed the title Bl 1829 update digital collections results on bento page BENTO ENGINE Bl 1829 update digital collections results on bento page Feb 21, 2024
@dkinzer
Copy link
Member

dkinzer commented Feb 22, 2024

OK, there's a lot going on here and because of that it's really hard to process for me. It almost seems like this is a new engine altogether and not just a small update to the cdm work. That said, I guess one thing that jumps out at me is copying the BentoSearch::ResultItem class wholesale into our code. It's hard to see what we added that's new and why. I don't think that's the way to go. Reading the Bento Search wiki on how to make custom field changes they provide several options that would be more inline with the framework. I would stay in the confines of what the framework allows if we are going to continue to use the framework: https://github.com/jrochkind/bento_search/wiki/Customizing-Results-Display#item-decorators-customizing-links-or-output-even-on-an-engine-by-engine-basis

@ebtoner
Copy link
Contributor

ebtoner commented Feb 23, 2024

@cdoyle-temple -- I pulled the branch down locally to review, and the basic layout in the bento in terms of the UI design looks fine to me. I will write up some next steps for the styling, but for now, please focus on the feedback that David provided in the previous comment. Let me know if there's anything that you'd like to discuss!

@cdoyle-temple
Copy link
Contributor Author

cdoyle-temple commented Feb 26, 2024

@dkinzer It is basically a rewrite of the engine. It was needed because the scope of this feature has changed from a simple total items lookup, and also the api endpoint at cdm is no longer the one previously used, and returns a different data structure than the previous query did. The additional fields are needed to be added to the ResultItem model in order to be accessed by the search controller and view partial. They don't need to be manipulated as in using a decorator, they need to be added to the model as separate custom fields first, and adding attr accessors to the model was the only I found to do that (lines 256 - 261).

@ebtoner
Copy link
Contributor

ebtoner commented Feb 26, 2024

@cdoyle-temple -- maybe you and I can meet to go over how we can modify some of the functional requirements for thia issue (ex. which fields display) to better align with what the existing bento search framework allows? I agree with David that it would be preferable to approach it that way if we can, and I'm willing to rescope this work accordingly.

@cdoyle-temple
Copy link
Contributor Author

@dkinzer We could also move this api request and display to a service object. I had mocked that up before finding out it was preferred to stay within the confines of the BentoSearch framework.

@cdoyle-temple
Copy link
Contributor Author

@ebtoner Sure, let's set up a meeting

@dkinzer
Copy link
Member

dkinzer commented Feb 26, 2024

@cdoyle-temple I think it's possible to do everything required for this feature request without overriding BentoSearch::ResultItem. A combination of the engine, decorator and templates is how you go about making it as custom as possible. And those are the tools the the framework gives to do overrides. That said, I am not going to push on this so I leave it up to @ebtoner and you to decide to move forward on it.

@sensei100
Copy link
Contributor

I agree that we need to figure out how to configure the bento search engine in a way that doesn't override the model. The gem is meant to be customizable and we have successfully used it with other sources, so I am confident that we can figure out a way. I'm happy to be part of the meeting if that would help.

@ebtoner
Copy link
Contributor

ebtoner commented Feb 27, 2024

@cdoyle-temple -- reviewing the default field options in the BentoSearch::ResultItem class, I think it's possible to use the following instead of custom cdm fields:
Instead of cdm_date, use publication_date
Instead of cdm_collection, use source_title
Instead of cdm_id, use unique_id
Instead of cdm_record_link, use link
Instead of cdm_thumbnail_link, use other_links

Let me know if this works!

@ebtoner ebtoner merged commit 4c3cfe6 into main Feb 27, 2024
2 checks passed
@ebtoner ebtoner deleted the BL-1829-update-digital-collections-results-on-bento-page-ENGINE branch February 27, 2024 18: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

4 participants