|
Description
David Cook
2012-11-21 21:05:57 UTC
Created attachment 13617 [details] [review] Bug 9130 - Remove old javascript from Parcel To test: 1) Go to Acquisitions 2) Find a vendor with more than 5 pending orders (or create enough new orders to put you over 5) 3) Click "Receive shipment" next to the name of that vendor 4) Type in an invoice number and go to the next page 5) Under "Pending Orders", you'll see a toolbar saying "Showing 1 to X of X". Beneath this toolbar, there will also be a line of text saying "Only the first 5 items are displayed. Click here to show all 'X' items." 6) Note that only 5 items are displayed in the table (as the text says) even though the toolbar says 1 to 10 of 11 or 1 to 6 of 6 or some such. 7) Apply the patch 8) Refresh your page 9) The line of text will be gone, and now the items displayed in the table will match the number mentioned in the toolbar. Created attachment 13640 [details] [review] [SIGNED-OFF] Bug 9130 - Remove old javascript from Parcel When receiving orders, the old javascript would show the following above the tables for pending orders and received orders: "Only the first 5 items are displayed. Click here to show all 'X' items." With the implementation of the Jquery Datatables plugin, this text and the 4 JS functions associated with it are now pointless (since users should be using the datatable toolbar) and problematic. The problem is that the Jquery toolbar says "Showing 1 to 10 of X", while in fact it is actually only showing 5 items, because that other JS takes place after the Jquery. While not a crippling bug, it's certainly irritating and misleading. Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> (In reply to comment #2) > While not a crippling bug, it's certainly irritating and misleading. Yes it is! Thank you for the patch ;) I add Kyle to the CC list of this bug. Do you agree with the current behaviour ? (In reply to comment #4) > I add Kyle to the CC list of this bug. > > Do you agree with the current behaviour ? I would tend to agree with you here. I do have one question. Does the table have a pulldown to choose the number of results displayed? If so, can we make that 'sticky' via a cookie instead. That way, the functionality isn't totally lost. I don't have a Koha installation set up for me to look at right now, or I could probably answer the question myself. Created attachment 13866 [details] [review] Bug 9130 follow up: add statesave cookie and "all" option for parcel drop-down menu This patch sets the bStateSave parameter for the "Pending orders" and "Already received" Datatables in Acquisitions, which uses a cookie to save the "pagination information, display length, filtering and sorting". It also sets the iCookieDuration parameter to 1000 days (the default value is 2 hours otherwise). It also adds an "all" option to the dro-down menu using the aLengthMenu parameter. -- Kyle, this should preserve that "sticky" functionality of which you spoke :) Fantastic! Good work! Kyle (In reply to comment #6) > Created attachment 13866 [details] [review] > Bug 9130 follow up: add statesave cookie and "all" option for parcel > drop-down menu > > This patch sets the bStateSave parameter for the "Pending orders" and > "Already received" Datatables in Acquisitions, which uses a cookie to save > the "pagination information, display length, filtering and sorting". > > It also sets the iCookieDuration parameter to 1000 days (the default value > is 2 hours otherwise). > > It also adds an "all" option to the dro-down menu using the aLengthMenu > parameter. > > -- > > Kyle, this should preserve that "sticky" functionality of which you spoke :) Created attachment 13870 [details] [review] [SIGNED-OFF] Bug 9130 follow up: add statesave cookie and "all" option for parcel drop-down menu This patch sets the bStateSave parameter for the "Pending orders" and "Already received" Datatables in Acquisitions, which uses a cookie to save the "pagination information, display length, filtering and sorting". It also sets the iCookieDuration parameter to 1000 days (the default value is 2 hours otherwise). It also adds an "all" option to the dro-down menu using the aLengthMenu parameter. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 13873 [details] [review] Bug 9130 - Remove old javascript from Parcel When receiving orders, the old javascript would show the following above the tables for pending orders and received orders: "Only the first 5 items are displayed. Click here to show all 'X' items." With the implementation of the Jquery Datatables plugin, this text and the 4 JS functions associated with it are now pointless (since users should be using the datatable toolbar) and problematic. The problem is that the Jquery toolbar says "Showing 1 to 10 of X", while in fact it is actually only showing 5 items, because that other JS takes place after the Jquery. While not a crippling bug, it's certainly irritating and misleading. Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Created attachment 13874 [details] [review] Bug 9130 follow up: add statesave cookie and "all" option for parcel drop-down menu This patch sets the bStateSave parameter for the "Pending orders" and "Already received" Datatables in Acquisitions, which uses a cookie to save the "pagination information, display length, filtering and sorting". It also sets the iCookieDuration parameter to 1000 days (the default value is 2 hours otherwise). It also adds an "all" option to the dro-down menu using the aLengthMenu parameter. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Thanks David for your followup. 1 little remark: when you add a value (60*60*24*1000), could you add directly the result (86400000)? Because like this the calculation is not performed each time. It is JS code so it is not crucial and not blocker. Marked as Passed QA. Created attachment 13875 [details] [review] Bug 9130 - Remove old javascript from Parcel When receiving orders, the old javascript would show the following above the tables for pending orders and received orders: "Only the first 5 items are displayed. Click here to show all 'X' items." With the implementation of the Jquery Datatables plugin, this text and the 4 JS functions associated with it are now pointless (since users should be using the datatable toolbar) and problematic. The problem is that the Jquery toolbar says "Showing 1 to 10 of X", while in fact it is actually only showing 5 items, because that other JS takes place after the Jquery. While not a crippling bug, it's certainly irritating and misleading. Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 13876 [details] [review] Bug 9130 follow up: add statesave cookie and "all" option for parcel drop-down menu This patch sets the bStateSave parameter for the "Pending orders" and "Already received" Datatables in Acquisitions, which uses a cookie to save the "pagination information, display length, filtering and sorting". It also sets the iCookieDuration parameter to 1000 days (the default value is 2 hours otherwise). It also adds an "all" option to the dro-down menu using the aLengthMenu parameter. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> This patch has been pushed to master. (In reply to comment #11) > Thanks David for your followup. > > 1 little remark: when you add a value (60*60*24*1000), could you add > directly the result (86400000)? Because like this the calculation is not > performed each time. > It is JS code so it is not crucial and not blocker. > > Marked as Passed QA. Good point, Jonathan. In retrospect, I could've just added the result. When looking through other examples (like in jquery.cookie.min.js), I noticed them writing out the whole calculation, so I just copied that at the time. I don't know if it's worth a follow-up, but certainly something to keep in mind for future Datatable mods. |