Streamline multisearch widget plugin - #212
Conversation
We retired the EBSCO Discovery Service some years ago, but never removed it as an option from the multisearch plugin until now. As we are simplifying this plugin significantly, the first step is to get rid of these templates.
This also removes the jQuery plugin that drives the tabbed search interface, because we are no longer supporting multiple search options in this widget. We also bump the plugin version to 1.8.0. Squash into alma removal
This reduces and combines the remaining conditional checks in the widget method, based on there now only being two template options. All conditionals are expressed in positive terms, as we no longer need to guard against "not this version" type checks.
It no longer makes sense to maintain two stylesheets for this widget, as we are attempting to simplify the styles with an eye toward removing as many of these styles as possible.
This is also a holdover from the EDS days, implemented prior to our pulling the search widget out to a separate plugin.
There was a problem hiding this comment.
Pull request overview
This PR streamlines the multisearch widget around Unified Search v1/v2 and removes legacy search implementations.
Changes:
- Removes EDS/Alma templates and legacy theme search scripts.
- Removes responsive-tabs assets and consolidates widget styling.
- Simplifies widget rendering and updates plugin assets.
Outstanding findings include restoring a direct jquery dependency, migrating or safely handling saved eds/alma settings, and retaining or migrating /search page behavior.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Summary |
|---|---|
web/app/themes/mitlib-parent/js/search.js |
Removes legacy search-page behavior. |
web/app/themes/mitlib-parent/js/search-ie.js |
Removes old IE search support. |
web/app/themes/mitlib-parent/functions.php |
Removes legacy search enqueues; /search behavior requires migration or retention. |
web/app/plugins/mitlib-multisearch-widget/templates/tab-more-eds.php |
Removes EDS “More” template. |
web/app/plugins/mitlib-multisearch-widget/templates/tab-more-alma.php |
Removes Alma “More” template. |
web/app/plugins/mitlib-multisearch-widget/templates/tab-books-eds.php |
Removes EDS books template. |
web/app/plugins/mitlib-multisearch-widget/templates/tab-books-alma.php |
Removes Alma books template. |
web/app/plugins/mitlib-multisearch-widget/templates/tab-articles-eds.php |
Removes EDS articles template. |
web/app/plugins/mitlib-multisearch-widget/templates/tab-articles-alma.php |
Removes Alma articles template. |
web/app/plugins/mitlib-multisearch-widget/templates/tab-all-use.php |
Removes obsolete debug markup. |
web/app/plugins/mitlib-multisearch-widget/templates/tab-all-use-v2.php |
Removes obsolete debug markup. |
web/app/plugins/mitlib-multisearch-widget/templates/tab-all-eds.php |
Removes EDS unified-search template. |
web/app/plugins/mitlib-multisearch-widget/templates/tab-all-alma.php |
Removes Alma unified-search template. |
web/app/plugins/mitlib-multisearch-widget/mitlib-multisearch-widget.php |
Updates plugin version and include syntax. |
web/app/plugins/mitlib-multisearch-widget/mitlib-multisearch-widget.js |
Removes responsive-tabs initialization. |
web/app/plugins/mitlib-multisearch-widget/mitlib-multisearch-widget.css |
Inlines responsive-tab styles. |
web/app/plugins/mitlib-multisearch-widget/libs/responsive-tabs.css |
Removes obsolete stylesheet. |
web/app/plugins/mitlib-multisearch-widget/libs/jquery.responsiveTabs.min.js |
Removes obsolete JavaScript library. |
web/app/plugins/mitlib-multisearch-widget/class-multisearch-widget.php |
Simplifies rendering; requires a direct jquery dependency and legacy-settings handling. |
Suppressed comments (3)
web/app/plugins/mitlib-multisearch-widget/class-multisearch-widget.php:73
- After the tab navigation was removed, this block no longer emits an element with id
tab-all, but the#search-allcontainer still references that id througharia-labelledby. This leaves an invalid accessible-name reference; point it at the existingsearchtabsheaderelement or remove the attribute.
if ( 'use' == $instance['targets'] ) {
web/app/plugins/mitlib-multisearch-widget/class-multisearch-widget.php:40
- This now defaults blank/legacy instances to
tab-all-use-v2, but that template's layout is provided by the theme'sv2stylesheet, which is only enqueued fortemplates/page-home-v2.php(web/app/themes/mitlib-parent/functions.php:262-268). The samesidebar-searchwidget is rendered by the standardpage.phptemplate, so a newly configured/default widget will appear there without the#search-form/.form-wrapperstyling. Restrict the v2 default to the v2 layout or enqueue the required styles wherever this variant is rendered.
// By default, we use the v2 template (suitable for v2 page layouts).
$all_template = 'templates/tab-all-use-v2.php';
if ( 'use' == $instance['targets'] ) {
web/app/plugins/mitlib-multisearch-widget/mitlib-multisearch-widget.js:33
- The default
use-v2template renders#basic-search-main, not#searchinput-bento. On that template this selector matches no elements, so.val()returnsundefinedand.trim()throws during document ready. Guard this call or bind it to the v2 input/button selectors.
preventSearch( '#searchinput-bento', '.search-bento .button-search');
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| plugin_dir_url( __FILE__ ) . 'mitlib-multisearch-widget.js', | ||
| array( 'responsivetabs-js' ), | ||
| '1.4.1', | ||
| array(), |
There was a problem hiding this comment.
The pages containing these widgets will have jQuery available, as the themes we use all have declared jQuery as a dependency. I'm not convinced that explicitly naming it as a dependency in this way is necessary, but I'm open to doing so if you'd like. Doing this doesn't break anything in my local, so there could be value is making it explicit.
Looking at the javascript that still remains in this plugin, I'm probably more interested in refactoring it so that we no longer rely on jQuery at all in this plugin - the selection rules that we're using seem like something that vanilla javascript is now capable of handling - but I don't want to expand the scope of this cleanup work to that extent right now.
There was a problem hiding this comment.
Yep, I'm fine with NOT declaring jQuery as dependency here since it's included on our themes.
+1 to refactoring jQuery out of this in the future... selection in vanilla is very similar nowadays and even the benefits of jQuery (animations/transitions) isn't that challenging to do with JS/CSS anymore. No need to worry about that now.
There was a problem hiding this comment.
There is quite a bit of javascript maintenance still do around this codebase, so it probably makes sense to ensure that we have that included on any platform roadmap that we come up with.
| // By default, we use the v2 template (suitable for v2 page layouts). | ||
| $all_template = 'templates/tab-all-use-v2.php'; | ||
| if ( 'use' == $instance['targets'] ) { |
There was a problem hiding this comment.
Handling legacy settings would be a legitimate concern if this code was part of a plugin that was being shipped to external consumers. Under those conditions, I absolutely agree that handling outdated values during a transition would be essential.
This codebase, however, is meant only for our public website - which no longer has any widgets configured for these settings.
| wp_enqueue_script( 'parent-search' ); | ||
| } | ||
|
|
||
| if ( in_category( 'has-menu' ) ) { |
There was a problem hiding this comment.
This recommendation doesn't make a lot of sense. The /search path is built into WordPress, so we can't remove it if we tried. Nor is this PR intending to remove that path - which will still exist as a piece of content after this PR merges. The effect of the change will be that the javascript we're removing - which relies on markup that was long ago removed from this codebase - will no longer be uselessly appended to that page, where it can do nothing.
djanelle-mit
left a comment
djanelle-mit
left a comment
There was a problem hiding this comment.
This looks good to me!
All permutations of the form work as expected on the multidev, the new logic in the plugin class is reasonable, and the JS / CSS seems to be preserved despite all the removed code. So nice being able to remove so much old code!!
Developer
Stylesheets
string incremented.
Secrets
Documentation
Accessibility
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Stakeholder approval
Dependencies
YES | NO dependencies are updated
Code Reviewer
(not just this pull request message)