search

projectblacklight / blacklight

Blacklight provides a discovery interface for any Solr (http://lucene.apache.org/solr) index.
http://projectblacklight.org/
Other
757 stars 255 forks source link

Use the new slot API for the modal #3120

Closed jcoyne closed 5 months ago

jcoyne commented 5 months ago

Closes #3102 and #3106

jrochkind commented 5 months ago

Oops, needed to actually work with ViewComponent 3.x, so a bug fix, yes?

jcoyne commented 5 months ago

@jrochkind yes, this is largely the same as https://github.com/projectblacklight/blacklight/pull/3106 but with the build passing (mostly)

jrochkind commented 5 months ago

Thanks! The difference between #3106 and this seems to be this one lacks additional specs.

Since the tests are being run with ViewComponent 3.x (yes?), which this func definitely didn't work with prior to this fix... the fact that we had green builds WITHOUT fixing #3102 definitely suggests we are missing some test coverage. So ideally we would add test coverage that would be red without this fix.

But if it's infeasible to provide for some reason, I guess better to have a fix that isn't covered by a test than no fix at all, I could approve anyway if it's infeasible to cover with test...

...but probably someone should at least manually confirm this really does solve #3102 and doesn't have any typos or whatever by manually exersizing func and confirming here they have?

jcoyne commented 5 months ago

@jrochkind I've added some tests here.

jrochkind commented 5 months ago

Curious why #3106 didn't pass, where it differed, but approved!