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

Refactor BlacklightHelper into a DocumentPresenter. Fixes #782 #787

Merged
merged 1 commit into from
Feb 19, 2014

Conversation

jcoyne
Copy link
Member

@jcoyne jcoyne commented Feb 18, 2014

No description provided.

@jcoyne
Copy link
Member Author

jcoyne commented Feb 18, 2014

This could look a lot nicer if it weren't for this commit: 855bec1 which introduced a document argument to some of the helpers. @cbeer do you remember why that was necessary?

@cbeer
Copy link
Member

cbeer commented Feb 18, 2014

I think I like it. I also like we didn't have to drag in draper or something pretty heavy-weight (although it looks like it'd be easy enough to use draper if you wanted, right?)

The document argument is an extension point for applications to make document-specific decisions about (generally) configuration. So, you might have one set of fields to render if your document is an X, but a completely different set if it is a Y. From #523:

Update index_field and document_show_field (and similar) methods to support an optional 'document' argument, which may be used downstream to inject alternative index/show fields based on characteristics of a document.

@jrochkind
Copy link
Member

I like providing a way to make document-specific decisions about things. Including hetereogenous documents in the same result set, that should potentially be displayed differently, is a key use case for us (even though we haven't really gotten around to doing it yet, it's always been part of our interest in BL).

@cbeer
Copy link
Member

cbeer commented Feb 18, 2014

@jcoyne what gets better if we don't have that optional document argument? Should we merge this now and make it better later, or is there something we could do now to make it closer to the ideal?

@jcoyne
Copy link
Member Author

jcoyne commented Feb 19, 2014

Merge now. We can refine further later. The improvement I had in mind would just be to do a straight delegation to the presenter, rather than making a new presenter for each call to the methods in BlacklightHelper.

cbeer added a commit that referenced this pull request Feb 19, 2014
Refactor BlacklightHelper into a DocumentPresenter. Fixes #782
@cbeer cbeer merged commit d5c3bf0 into master Feb 19, 2014
@cbeer cbeer deleted the document_presenter branch February 19, 2014 01:49
@cbeer cbeer added this to the 5.2.0 milestone Mar 10, 2014
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

3 participants