Extract out overrideable Catalog#display_home_content? method

[jrochkind]

It was previously hard-coded into catalog/index.html.erb that if you has_search_parameters?, you display search results, and otherwise you display "home" splash content.

But my app never wants to display home splash content. It wants to always display search results (with no search parameters, that would be: all results).

Other apps may want to have different criteria for when to display home splash screen results.

By making the logic rely on new extracted display_home_content? method, we provide an override hook point to easily customize this choice without having to override the template (with it's future compatibility and maintainance issues).

The default defintion of display_home_content? is !has_search_parameters? for complete backwards compatibility with previous logic.

[jcoyne]

I wonder if display_splash_content? is a better name. I think that "No results found" could be classified under "home content", which is not what we want here. Thoughts?

[jcoyne]

Can we move this into Blacklight::BlacklightHelperBehavior or maybe a ViewComponent? I don't see any reason this should be in a controller.

[jrochkind]

Happy to call it whatever anyone wants -- "home content" is what the source code seemed to refer to it as before? The partial it displays is called home, not splash, and changing that would not be backwards compat.

But display_splash_content? works for me if you prefer it.

I don't think I understand how to put it in a ViewComponent, within the constraints that: the whole purpose here is that I want to be able to easily change this logic WITHOUT having to copy-paste large swaths of template source code to do so; and I don't understand a reasonable view component to introduce here.

But I can try putting it in Blacklight::BlacklightHelperBehavior, sure!

[jrochkind]

Thanks for quick feedback @jcoyne , I changed the method name and location trying to respond to your requests!

[jrochkind]

I guess there's still a way for local app to easily override, even when it's in module Blacklight::CatalogHelperBehavior?

That's the only reason I originally put it in the controller, it was clear to me how to override.

I guess one way override helper method in helper module, is still with a controller method and helper in controller? Is there a better way?

[jrochkind]

I do not understand the failures, I don't think any of them have anything to do with my changed code? Pre-existing failures on main?

Failures involving Blacklight::FacetItemPivotComponent that I can't think of any way my change could cause? Linting failures on source i didn't touch?

I'm not sure, if anyone understands better than me what's going on here, advice welcome.

[barmintor]

I like the idea of making this configurable, but I dread a little using a helper override to do it - in part because we don't want to encourage relying on helper overrides in general!

How would people feel about a blacklight_config.view_config(:home).partials list being rendered there? That's closer to the pattern of the other actions, and would let an installation effectively suppress it simply by configuring it to use the search results partial.

[barmintor]

To be clear, I totally agree with @jcoyne that it's not desirable to have this in the controller mixin; I would just go a step further and say it's better to use configuration over a helper method as well.

[jrochkind]

I have already refactored as a helper, in the specific catalog-controller-helper module, rather than generic BlacklightHelperBehavior.

I think it is beyond my current capacity to refactor as config, that would take a lot more work.

If that's a blocker, and I'm not available to do that work myself, would it be better to close this ticket @barmintor ?

I think this is probably a useful use case not limited to me -- I'm currently having to copy-paste-override a pretty large swath of functionality to get it, which annoys me. But I just don't have the capacity to figure out how to do it as configuration, it's not clear to me how to do that and I'm not very familiar with that area of BL architecture. I guess the configuration would have to be a lambda/proc value?

[jrochkind]

Sorry @barmintor, I see now your specific suggestion blacklight_config.view_config(:home).partials I'm afraid I don't underestand how this specific suggestion can accomplish what is needed (which isn't just a flat list of partials whose rendering is concatenated, but involves logic for when to show which partial), and don't really have capacity to work on it.

[barmintor]

@jrochkind what I am suggesting is that another way to approach this, rather than "I need a way to say not to display the home partials", is to provide a way to configure the home partials to be the same as the index partials. I'm not requiring you to do this, but I would prefer to implement things that way if it addresses the use case. There's not an associated ticket for this yet, is there?

[barmintor]

I would like to discuss alternative approaches to an overridable helper for this use case.

[jrochkind]

@barmintor I don't believe there is an associated ticket!

I don't follow how a blacklight_config.view_config(:home).partials solves the use case, since the current implementation involves logic to determine which of two rendering paths to take. I don't understand how you replace that with a flat list of 0 to N partials.

So where I'd go to, if it had to be config rather than helper, is a config that's takes a lambda value to replace the current hard-coded conditional.

But that's fine, you may have a plan I'm just not following!

I wish there were a simpler way to PR without having to dig into and understand the config system, but I understand where you are coming from. the bar for BL PR's from outsiders seems high right now, but there are reasons.

[barmintor]

<% unless has_search_parameters? %>
  <%# if there are no input/search related params, display the "home" partial -%>
  <%= render 'home' %>
  <%= render 'shared/sitelinks_search_box' %>
<% else %>
  <%= render 'search_results' %>
<% end %>

Might be changed to:

<% unless has_search_parameters? %>
  <%# if there are no input/search related params, display the "home" partial -%>
  <%= render blacklight_config.view_config(:home).template %>
  <%= render 'shared/sitelinks_search_box' %>
<% else %>
  <%= render 'search_results' %>
<% end %>

  configure_blacklight do |config|
    config.home.template = 'search_results'
  end

[barmintor]

Or something similar - I'd like to get some feedback (and ask whether shared/sitelinks_search_box should move into the home partial while we're at it).

[jrochkind]

@barmintor

I want the search results even if has_search_parameters? is false.

To show un-limited/un-qualified all corpus.

So my use case is resolved if I have some way to customize the logic currently hard-coded as has_search_parameters?, but is not, I think, unless I'm missing it, resolved by your suggestion above (thanks for spelling it out).

I want to eliminate the check for has_search_parameters? in my customized app -- I would redefine it as true. Or really probably reverse the meaning to be able to redefine it as false, as I tried to do in PR.

Another answer could be this is not a common enough use case for Blacklight to support. I am currently over-riding all of catalog/index.html.erb in order to be able to do what I need. Perhaps that is sufficient. But when it seemed like only a few lines of entirely backwards-compat logic could get me what I needed, I thought I'd test the waters with a PR.

[barmintor]

@jrochkind in the example I posted you do not need to customize the logic currently hard-coded as has_search_parameters? because the configured partial is search_results

[jrochkind]

Ohhhh I see. Interesting, I might not have not figured out how to do that customization without that hint on my own, it's a non-obvious use of that config to me, but I see what you mean now, sure it could work!

If you have reason to think people would want to use this customization to actually add different partials there, that could be reasonable. I don't know of any such use cases myself. It seems like a convoluted way to meet my use case, but that's just my read.

[barmintor]

@jrochkind just for expediency, I want to note re: I'm currently having to copy-paste-override a pretty large swath of functionality to get it, which annoys me.... As much as I hate to suggest partial overrides, you could probably just have a one line catalog/_home.html.erb that accomplished this with:

<%= render :partial=>'search_results' %>

Right?

[jrochkind]

@barmintor interesting, yeah, similar to your config suggestion -- it hadn't occured to me to override just the "home content" path to make it a duplicate of the non-home-content path.

I guess I've trained myself out of such non-DRY ways, but you make a good suggestion, I'll consider that as an override if a targetted supported method is not available.

[jrochkind]

As the original also displays a shared/sitelinks_search_box only in the case of not has_search_parameters? (I really don't understand what this is, or why it is displayed only in that case?), I might also have to override that one to empty, if I want both paths to be identical.

It's a somewhat confusing set of overrides to me that I'm not sure if it will be more or less maintainable in the end, but it's def worth considering, thanks for the suggestion.

blacklight/app/views/catalog/index.html.erb

Lines 11 to 17 in 35d584a

	<% unless has_search_parameters? %>
	<%# if there are no input/search related params, display the "home" partial -%>
	<%= render 'home' %>
	<%= render 'shared/sitelinks_search_box' %>
	<% else %>
	<%= render 'search_results' %>
	<% end %>

[barmintor]

shared/sitelinks_search_box is meta information about the search widget that you will want on the home page regardless; it's a <script> and doesn't display anything.

[jrochkind]

@barmintor To me, it feels like being able to use a CatalogController-type controller (some apps have more than one), without this special "home content" rendering seems like a good BL feature -- like there are so many things you might want to use a CatalogController-type controller for, it's the basic unit of Blacklight, and the "home" rendering pathway is what seems like a "special" feature to me, not one you should be hard-coded forced to use with every use of CatalogController.

That is part of my motivation for PR'ing it, but others may not share that evaluation.

[barmintor]

It doesn't seem unusual to me to want to display something different when there are no search parameters - and for that matter, yet another way to handle this would be to set a default value for search field in the application routes, or in the controller. But I'm also not hostile to the idea, I'm just wary of building in an expectation of helper overrides when we have had so much work fall out of the deprecation of helper methods in the past.

[jrochkind]

it's not that it's unusual (obviously not), it's that in the whole domain of anything you might want to use a CatalogController for (the main thing Blacklight provides, a way of displaying search results), it also does not seem unusual to me to want the more basic case of not doing that extra logic path.

It seems to me there are many use cases where you would not want it, especially once you realize it may not be at the "root" page of the app (unlike blacklight default), there are all sorts of places you might wantt use it. In my app the CatalogController is not the "root" page of the app. It seems like a feature mainly intended for when the CatalogController is the root page of the app -- which is not unusual, agreed.