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

Add a slot for rendering extra inputs in the search bar #2864

Merged
merged 1 commit into from Nov 8, 2022
Merged

Add a slot for rendering extra inputs in the search bar #2864

merged 1 commit into from Nov 8, 2022

Conversation

thatbudakguy
Copy link
Member

Refs #2862 and #2863

@thatbudakguy thatbudakguy marked this pull request as ready for review November 1, 2022 22:20
@jcoyne
Copy link
Member

jcoyne commented Nov 2, 2022

@thatbudakguy what do you think about this approach instead? #2866 We could then have our own search navbar component rather than adding an extension point to this one. I think that we want to avoid adding arbitrary extension points to every component and encourage people to override components instead.

@thatbudakguy
Copy link
Member Author

I think #2866 is sensible (and might help us for the reasons you mention), but I don't think it solves the problem in arclight that this PR is intended to solve — or at least not fully, anyway.

Arclight currently renders:

<div class="navbar-search navbar">
  <div class="container">
    <form class="search-query-form">
      <div class="input-group"> <!-- input group for the search input -->
        <div class="input-group"></div> <!-- input group for the extra dropdown arclight adds -->
      </div>
    </form>
  </div>
</div>

To make it more correct vis-a-vis bootstrap, and to solve issues like projectblacklight/arclight#1239 that need the appropriate HTML structure, we instead want:

<div class="navbar-search navbar">
  <div class="container">
    <form class="search-query-form">
      <div class="input-group"></div> <!-- input group for the extra dropdown arclight adds -->
      <div class="input-group"></div> <!-- input group for the search input -->
    </form>
  </div>
</div>

The issue is that blacklight's SearchBarComponent actually includes the entire <form>, so if you want to add more inputs to it, you need to either override the whole component, or use a slot. #2866 definitely helps solve the problem of adding more content above/below the search bar, but it doesn't address this narrower use case of making sure that more inputs end up (as siblings) inside the search form.

That said, I very much agree with:

I think that we want to avoid adding arbitrary extension points to every component and encourage people to override components instead.

So maybe overriding the entire SearchBarComponent is the way to go regardless.

@cbeer cbeer merged commit 598369c into projectblacklight:main Nov 8, 2022
@thatbudakguy thatbudakguy deleted the searchbar-input-slots branch November 8, 2022 20:25
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