Created attachment 20837 [details] Screenshot of the revised interface I propose to change the layout of the OPAC place hold screen so that information is not displayed in a table. As options are added to the place hold process the table layout gets more an more unwieldy. This revised version makes the layout feel more open and provides a way to show and hide the finer hold controls.
Created attachment 20920 [details] [review] Bug 10836 - New layout for placing holds in the OPAC This patch changes the layout of the OPAC's place hold screen, eliminating the table in favor of a more flexible, open display. To test, place single and multiple holds under various conditions: - Items which are not available to be place on hold - With OpacHoldNotes on and off - With OPACItemHolds on and off - With OPACAllowHoldDateInFuture on and off - With IndependentBranches on and off - With JavaScript disabled Test setting hold notes, holding specific copies, and setting hold start dates. When setting a hold start date and hold expiration date, the two datepickers should work together to prevent invalid date combinations (dates after today, expiration dates which precede the start date).
Created attachment 21412 [details] [review] [SIGNED-OFF] Bug 10836 - New layout for placing holds in the OPAC This patch changes the layout of the OPAC's place hold screen, eliminating the table in favor of a more flexible, open display. To test, place single and multiple holds under various conditions: - Items which are not available to be place on hold - With OpacHoldNotes on and off - With OPACItemHolds on and off - With OPACAllowHoldDateInFuture on and off - With IndependentBranches on and off - With JavaScript disabled Test setting hold notes, holding specific copies, and setting hold start dates. When setting a hold start date and hold expiration date, the two datepickers should work together to prevent invalid date combinations (dates after today, expiration dates which precede the start date). Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
All good! :-) Tested with different sysprefs configurations (not tested with IndependentBranches on) Datepickers work as they should. The only little thing I have to say, is when Javascript is disabled, you can pick option "A specific copy" without choosing a copy in the list of items, and then submit. No errors appear and the hold is placed on the next available copy. But I think this should not prevent patch to be pushed.
Congratulations Owen for your patience! Indeed, this code is really dirty and hard to change. There are some inconsistencies, but not introduced by this patch. It improves the ergonomics of the page and I didn't find any regression. Since the patch does not affect core routines, there is no chance to find a big issue later. Marked as Passed QA.
Created attachment 21416 [details] [review] Bug 10836 - New layout for placing holds in the OPAC This patch changes the layout of the OPAC's place hold screen, eliminating the table in favor of a more flexible, open display. To test, place single and multiple holds under various conditions: - Items which are not available to be place on hold - With OpacHoldNotes on and off - With OPACItemHolds on and off - With OPACAllowHoldDateInFuture on and off - With IndependentBranches on and off - With JavaScript disabled Test setting hold notes, holding specific copies, and setting hold start dates. When setting a hold start date and hold expiration date, the two datepickers should work together to prevent invalid date combinations (dates after today, expiration dates which precede the start date). Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
One observation during testing: - From search results, select multiple records and click the place hold button - Select the "a specific copy" option for one of the titles. - Select the "a specific copy" option for another one -- instead of it displaying the copies for that title, it instead display an alert whose text is "Expecting a specific copy selection". I wouldn't expect that alert to occur prior to attempting to place the holds.
(In reply to Galen Charlton from comment #6) > One observation during testing: > > - From search results, select multiple records and click the place hold > button > - Select the "a specific copy" option for one of the titles. > - Select the "a specific copy" option for another one -- instead of it > displaying the copies for that title, it instead display an alert whose text > is "Expecting a specific copy selection". > > I wouldn't expect that alert to occur prior to attempting to place the holds. Further testing shows, however, that this was occurring before the patch.
Comment on attachment 21416 [details] [review] Bug 10836 - New layout for placing holds in the OPAC Review of attachment 21416 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/opac-tmpl/prog/en/modules/opac-reserve.tt @@ +184,5 @@ > + var optionsID = this.id.replace("toggle-hold-options-",""); > + var copiesRow = $("#copiesrow_"+optionsID) > + $("#hold-options-"+optionsID).toggle(0, function() { > + if( copiesRow.is(':visible') ){ > + $("#reqany_"+optionsID).prop("checked",true); I'm not sure about this -- if you expand options, choose to select a specific copy, then hide the options (say, because you're placing several holds at once and want to reclaim screen real estate), the request type gets silently changed to a title hold. Is there a reason to do that?
(In reply to Galen Charlton from comment #8) > I'm not sure about this -- if you expand options, choose to select a > specific copy, then hide the options (say, because you're placing several > holds at once and want to reclaim screen real estate), the request type gets > silently changed to a title hold. Is there a reason to do that? Looks like moving the copy table into the hold options div eliminates the need for special code to hide the copy table.
Created attachment 21517 [details] [review] Bug 10836 - New layout for placing holds in the OPAC This patch changes the layout of the OPAC's place hold screen, eliminating the table in favor of a more flexible, open display. To test, place single and multiple holds under various conditions: - Items which are not available to be place on hold - With OpacHoldNotes on and off - With OPACItemHolds on and off - With OPACAllowHoldDateInFuture on and off - With IndependentBranches on and off - With JavaScript disabled Test setting hold notes, holding specific copies, and setting hold start dates. When setting a hold start date and hold expiration date, the two datepickers should work together to prevent invalid date combinations (dates after today, expiration dates which precede the start date). Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Created attachment 21518 [details] [review] Bug 10836: (follow-up) improve display toggles on the OPAC hold request form This patch improves the the toggles for displaying and hiding the hold options div and the toggles for displaying the table of specific copies to request. It also moves the copy table into the hold options div. To test: [1] Select a single title in the OPAC and click on the place hold button. [2] Click 'Show more options', and select a specific copy to request. [3] Click 'Hide options'. Verify that the hold options, including the copy table, are hidden. [4] Click 'Show more options'. Verify that the specific copy radio button is still selected and that the copy selected in step to is selected. [5] Enable DisplayMultiPlaceHold and select several titles to request. [6] Do steps 2, 3, and 4 on various titles on the request form. Verify that toggling the options link for one title on the form affects the visibility only for that title. Signed-off-by: Galen Charlton <gmc@esilibrary.com>
I've attached a follow-up that addresses my concerns with the display toggling. I want another pair of eyes on the follow-up; either a QA person or Owen can do it.
Created attachment 21519 [details] [review] [SIGNED-OFF] Bug 10836: (follow-up) improve display toggles on the OPAC hold request form This patch improves the the toggles for displaying and hiding the hold options div and the toggles for displaying the table of specific copies to request. It also moves the copy table into the hold options div. To test: [1] Select a single title in the OPAC and click on the place hold button. [2] Click 'Show more options', and select a specific copy to request. [3] Click 'Hide options'. Verify that the hold options, including the copy table, are hidden. [4] Click 'Show more options'. Verify that the specific copy radio button is still selected and that the copy selected in step to is selected. [5] Enable DisplayMultiPlaceHold and select several titles to request. [6] Do steps 2, 3, and 4 on various titles on the request form. Verify that toggling the options link for one title on the form affects the visibility only for that title. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> While signing off I removed one <br /> tag and corrected a float in the CSS. This helps keep the border between individual hold listings.
Pushed to master. Thanks, Owen!