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

Combined: Fix scope of search cart form #3219

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

maccabeelevine
Copy link
Member

@maccabeelevine maccabeelevine commented Nov 14, 2023

In search/results.phtml the form element
<form id="search-cart-form" ...
wraps just a single line, the call to render search/bulk-action.buttons.phtml.

In combined/results.phtml, the same form wraps most of the whole file. It looks like search/results.phtml used to do the same, but was refactored in #802.

This is a problem just now because after #3135 one of the things it wraps is side recommendations, which can include SideFacets.phtml, which can include range-slider.phtml, which defines its own form. It's invalid HTML to have a form nested within another form, and forces the browser to move the entire sidebar div outside of the search-cart-form. This in turn messes up the styling assumptions where div.mainbody and div.sidebar are both floated and assumed to be adjacent elements.

This PR changes combined/results.phtml so the form scope mirrors search/results.phtml.

@maccabeelevine
Copy link
Member Author

I am admittedly no expert on how the search cart form works. I turned on showBulkOptions in config.ini, and I was still able to use one of the bulk operations (export). Not sure what else to test.

@maccabeelevine maccabeelevine marked this pull request as ready for review November 14, 2023 16:41
@demiankatz demiankatz added this to the 10.0 milestone Nov 16, 2023
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @maccabeelevine, it definitely makes sense to fix the inconsistency between the combined and regular templates. I've confirmed that the full test suite passes and that manual testing of combined cart and bulk functionality reveals no problems after this change.

Even though this is a bug fix, it does not appear to cleanly backport to the release-9.1 branch due to other changes to combined search on the dev branch. Thus, I'm going to merge it to dev for inclusion in release 10.0; I think the situation where this will cause a problem is rare enough that the fix can wait for the major release. Of course, I'm happy to revisit that decision if anyone needs me to!

@demiankatz demiankatz merged commit d8d8fd6 into vufind-org:dev Nov 16, 2023
7 checks passed
@maccabeelevine maccabeelevine deleted the combined-search-cart-form branch November 16, 2023 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants