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

Use ViewComponent Slots v2 API #242

Merged
merged 1 commit into from
Oct 11, 2023
Merged

Use ViewComponent Slots v2 API #242

merged 1 commit into from
Oct 11, 2023

Conversation

jcoyne
Copy link
Member

@jcoyne jcoyne commented Oct 10, 2023

No description provided.

@jcoyne jcoyne force-pushed the view_component3 branch 2 times, most recently from 69b1951 to e8a725e Compare October 10, 2023 22:15
@jcoyne jcoyne marked this pull request as ready for review October 11, 2023 15:32
@jrochkind
Copy link
Member

I do not understand the extra_facet_fields conditional mocking for Blacklight 8 thing, don't understand what's going on. So don't love that I have no idea how I would have done this without jcoyne, or how to write similar tests in the future.

But am super grateful to @jcoyne for getting us to green, AND with viewcomponent 3 support! I was totally blocked on that and don't think I would have been able to figure it out literally ever.

I am going to go ahead and approve/merge this despite not totally following it, glad to be moving forward.

Note (Cc @barmintor ) as a result of #241, we are now only testing on BL7, even though the gem still allows BL8, for those who have been using it all along on BL8 despite it never passing tests on BL8.

@jrochkind jrochkind merged commit 6cffb2e into main Oct 11, 2023
6 checks passed
@jrochkind jrochkind deleted the view_component3 branch October 11, 2023 17:27
jrochkind added a commit that referenced this pull request Oct 12, 2023
With #242 to use Slots v2 API, we now require at least view_component 2.54.0, that's when it was introduced. https://viewcomponent.org/CHANGELOG.html#2540

I think we learned from #242 (and analagous things happening with other BL plugins) that if we're using ViewComponent API directly, as we are, we should declare a direct dependency on it, and what we know to work with, instead of just relying on indirect dependency from blacklight. So also adding a cap at , since a future 4 might include currently unknown backwards incompat changes that effect this code. I think this is just responsible dependency declaration.
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