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

"Limit your search" header shows even if no facets are available, eg a zero results page #3054

Open
jrochkind opened this issue Jul 5, 2023 · 6 comments

Comments

@jrochkind
Copy link
Member

This was not true in Blacklight 7.

But in Blacklight 8.0.1, see eg: https://demo.projectblacklight.org/?search_field=all_fields&q=notherenosuchthing


Screen Shot 2023-07-05 at 4 44 22 PM


In Blacklight 7.x, that extraneous and confusing "Limit your search" header would not have been there.

Looking into it, I'm a bit lost as to how to fix it... I think maybe FacetGroupComponent#render? would have to return false to avoid rendering this whole thing including title.

In Blacklight 8.0.1, the definition of FacetGroupComponent#render? is the somewhat indirect:

      def render?
        body.present?
      end

I'm not sure what would determine whether body was present or not, but it's definitely present even when there are no facets, as in above example.

In Blacklight 7.32.0 it was the somewhat more predictable:

      def render?
        Deprecation.silence(Blacklight::FacetsHelperBehavior) do
          helpers.has_facet_values?(@fields, @response)
        end
      end

With the new definition, I'm not sure how one would make it do what's desired/intended, while also not rendering when there is no facet info?

It looks like the change in render? definition was made quite a while ago, by @cbeer, in 89c635e / #2583

I wonder if @cbeer has any thoughts.

@barmintor
Copy link
Contributor

The body slot is always going to be present (in this case, a ViewComponent::Slot object), so that render? implementation will always return true.

What's the desired behavior here? If the component doesn't render at all, the layout is affected (which might be fine). If we want the component to render with a different (or no) label, we would need to stringify the body slot's content and see if it is present, and nil out or override the @title attribute. That would preserve the page layout, with empty space where the facet sidebar normally would be.

The body slot content is set by the sidebar component to be a collection of facet components, but a given application could also be setting it to string content, so I'm wary of inspecting (for example) @response&.aggregations or something, even though that would more closely mimic the 7.x behavior.

@jrochkind
Copy link
Member Author

@barmintor

      def render?
        body.present?
      end

Is the current implementation in main/Blacklight 8.0.1. Sorry if I'm repeating the obvious, I want to make sure we're talking about the same thing.

I agree, from observation, that it is not a useful implementation. Thus filing this ticket.

In Blacklight 7, in the default state where the sidebar only contains ordinary facets, this whole component does not render if none of the contained facets have contents.

In Blacklight 8, even if there are no contents, the FacetGroupComponent does still render, resulting in a header and empty div on the page.

My guess is that the intent of the current Blacklight 8 implementation is to preserve Blacklight 7 behavior, where if the content is empty, the component does not render at all.

It does not accomplish that.

As a result, on the "no results found" page in Blacklight 8, you get a very confusing "Limit your search" header (with nothing under it). You did not get this in Blacklight 7. It seems like a bug/undesirable change to me.

I do not know/have not been able to figure out a solution. I would like Blacklight 7 behavior back, where if there is no content to be displayed inside of it, then FacetGroupComponent does not dispaly itself with a header and empty div.

Does this answer your questions?

@barmintor
Copy link
Contributor

I understand that you don't want the header that says "Limit Your search". Whether the FacetGroupComponent renders at all is a related but separate implementation question that has an impact on layout. If the "normal" layout is to be preserved, the component needs to render an empty div. If the layout changing is acceptable, the component could not render anything. The Blacklight 7 implementation will not necessarily work, because the client code determines the content of the body slot for the component and it is not necessarily the default list of FacetComponent.

@jrochkind
Copy link
Member Author

jrochkind commented Jul 10, 2023

@barmintor OK, fair.

In Blacklight 7, the way that was solved was by FacetGroupComponent not rendering at all, due to FacetGroupComponent#render? implementation in BL7.

My assumption/inclination was to try to restore that for BL8, consider it as a regression on that particular behavior.

But there could be a different way to restore the same user-visible behavior in BL8, true.

It does seem like the current BL 8.0.1 FacetGroupComponent#render? is basically a no-op, and a mistake?

@barmintor
Copy link
Contributor

It does seem like the current BL 8.0.1 FacetGroupComponent#render? is basically a no-op, and a mistake?

The logic there could work if we did not provide the deprecated default body slot; I'm not sure when that deprecation went in (possibly it was intended to be removed in BL8, I haven' been able to look it up).

I think this could be addressed by calling to_s on the body slot to see if it is empty; this would not add overhead when it is non-empty but would add a small amount of overhead when it was.

@jrochkind
Copy link
Member Author

jrochkind commented Jul 10, 2023

I think this could be addressed by calling to_s on the body slot to see if it is empty; this would not add overhead when it is non-empty but would add a small amount of overhead when it was.

I think it is rarely empty in typical use cases -- I can't really identify any except the "zero results" case. (As the sidebar is typically facets, and any non-zero results will typically have at least one value in at least one facet).

So that might be a good solution that will solve my use case, with minimal changes to the code?

Now that tests are passing on main again, it might be a little bit easier to figure out how to add a test for this behavior and see if that change fixes it. I am not sure when I personally will have time to get back to it, so if anyone else is interested please feel free!

Thanks for that suggestion @barmintor !

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

No branches or pull requests

2 participants