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

Allow the view_config to be passed into the document_presenter helper and DocumentComponent constructor #3343

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cbeer
Copy link
Member

@cbeer cbeer commented Sep 27, 2024

This allows downstream applications (and e.g. gems like spotlight) a little more flexibility in how they use these.

@cbeer cbeer marked this pull request as ready for review September 27, 2024 20:06
@@ -137,6 +138,12 @@ def before_render
end
end

def view_config
@view_config ||= presenter&.view_config || Blacklight::Configuration::ViewConfig.new
Copy link
Member

Choose a reason for hiding this comment

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

Is there really a case where presenter is nil? Wouldn't we also need to deal with that in the delegate :field_presenters?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know. Someone thought so in all the renders_one blocks above, but not in the before_render.

@jcoyne
Copy link
Member

jcoyne commented Sep 27, 2024

Can you explain why downstream apps need more flexibility? In order to do what?

It seems that this would allow the presenter.view_config to be different than DocumentComponent#view_config and that seems like a lot of complexity.

@cbeer
Copy link
Member Author

cbeer commented Sep 27, 2024

Spotlight is a good example where its use of document presenters and components doesn't neatly align with Blacklight's limited notion of search results and single document pages, and where document_presenter_class is really a bad fit:

def document_presenter_class(_document = nil)
case action_name
when 'show', 'citation'
blacklight_config.view_config(:show, action_name: action_name).document_presenter_class
else
blacklight_config.view_config(document_index_view_type, action_name: action_name).document_presenter_class
end

@jcoyne
Copy link
Member

jcoyne commented Sep 27, 2024

@cbeer It seems like what Spotlight has right now is working... so what do you want it to look like?

@cbeer
Copy link
Member Author

cbeer commented Sep 27, 2024

Yes, spotlight, blacklight-gallery and who-knows-what-else could all provide their own document_presenter implementations to just get the document presenter instance they actually want. It seems like an anti-pattern to force each gem to do that when we could have a standardized implementation here for just a tiny change.

@jcoyne
Copy link
Member

jcoyne commented Sep 28, 2024

So the main thing here is the addition of view_config&.document_presenter_class in #document_presenter? That seems like a good change. Do you think we should deprecate the call to document_presenter_class helper method? Do we still need two ways to do this?

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.

2 participants