I would like to start working harder to keep JavaScript separated from the templates. I hope that in doing so we can have JavaScript which is more easily cached by the browser, more testable, and potentially able to be minified and/or concatenated with other JS files. To that end I have begun moving JavaScript out of the circulation template into a page-specific file, circulation.js. I hope that further improvements can be made, perhaps by moving some of the template variables required by the template into HTML5 data- attributes.
Created attachment 14154 [details] [review] Bug 9294 - Move JavaScript out of circulation template This patch creates a new js file, js/pages/circulation.js, and begins the process of moving JavaScript into it from circulation.tt. I have started the process by focusing on sections which are least dependent on template variables. To test, load a patron for checkout who has something checked out. There should be no JavaScript errors: - Tabs should work. - Showing and hiding the "Add message" form should work. - The "specify due date" datepicker should work. - Controls for selecting checkboxes in the checkouts table should work - With checkout exports enabled, warnings should be displayed if you have no checkouts selected, or if you try to export as CSV and have no CSV profile specified in your ExportWithCsvProfile.
Sorry, meant to point out: This patch depends on the one for Bug 9283.
Created attachment 14251 [details] [review] Bug 9294 - Move JavaScript out of circulation template This patch creates a new js file, js/pages/circulation.js, and begins the process of moving JavaScript into it from circulation.tt. I have started the process by focusing on sections which are least dependent on template variables. To test, load a patron for checkout who has something checked out. There should be no JavaScript errors: - Tabs should work. - Showing and hiding the "Add message" form should work. - The "specify due date" datepicker should work. - Controls for selecting checkboxes in the checkouts table should work - With checkout exports enabled, warnings should be displayed if you have no checkouts selected, or if you try to export as CSV and have no CSV profile specified in your ExportWithCsvProfile. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Oops sorry, this bug depends on Bug 9283
I prefer to wait before passing qa on this patch. It depends on another one not already pushed and it interferes with Bug 9289. On rebasing, we won't forget to uncomment this line!
Created attachment 14417 [details] [review] Bug 9294 - Move JavaScript out of circulation template This patch creates a new js file, js/pages/circulation.js, and begins the process of moving JavaScript into it from circulation.tt. I have started the process by focusing on sections which are least dependent on template variables. To test, load a patron for checkout who has something checked out. There should be no JavaScript errors: - Tabs should work. - Showing and hiding the "Add message" form should work. - The "specify due date" datepicker should work. - Controls for selecting checkboxes in the checkouts table should work - With checkout exports enabled, warnings should be displayed if you have no checkouts selected, or if you try to export as CSV and have no CSV profile specified in your ExportWithCsvProfile. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Fixed conflict with Bug 9289. Still waiting on Bug 9283, on which this patch depends.
QA comment: If ExportRemoveFields is filled and no csv profile defined in ExportWithCsvProfile, the csv choice appears in the dropdown list but does not work. I think we should remove it from the list if no profile is defined. Marked as Failed QA.
Created attachment 16127 [details] [review] Bug 9294: Followup FIX csv choice for export If ExportRemoveFields is filled and no csv profile defined in ExportWithCsvProfile, the csv choice appears in the dropdown list but does not work.
I think this quick followup fixes the issue, could you confirm?
Created attachment 16303 [details] [review] Bug 9294 [Follow-up] Move JavaScript out of circulation template This patch corrects a JavaScript error in the original patch caused by an incorrectly named variable. Instead of correcting the name I'm removing the error-handling around missing CSV profiles alltogether in favor of Jonathan's suggestion to simply hide the CSV option if no formats are specified. To test this follow-up perform exports of checkouts with ExportRemoveFields filled and with ExportWithCsvProfile both empty and not empty.
Owen, the first patch does not apply fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt). Could your rebase it please?
(In reply to comment #14) > Owen, the first patch does not apply > fatal: sha1 information is lacking or useless > (koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt). > Could your rebase it please? In fact it is the second one! fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/js/pages/circulation.js).
Created attachment 20382 [details] [review] Bug 9294 - Move JavaScript out of circulation template This patch creates a new js file, js/pages/circulation.js, and begins the process of moving JavaScript into it from circulation.tt. I have started the process by focusing on sections which are least dependent on template variables. To test, load a patron for checkout who has something checked out. There should be no JavaScript errors: - Tabs should work. - Showing and hiding the "Add message" form should work. - The "specify due date" datepicker should work. - Controls for selecting checkboxes in the checkouts table should work
Created attachment 20684 [details] [review] SIGNED OFF: Bug-9294-Move-JavaScript-out-of-circulation-template.patch Bug 9294 - Move JavaScript out of circulation template (14.29 KB, patch) 2013-01-04 14:11 UTC, Owen Leonard and Bug 9294 [Follow-up] Move JavaScript out of circulation template (3.16 KB, patch) 2013-03-18 18:26 UTC, Owen Leonard were unable to be applied error fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 9294 - Move JavaScript out of circulation template
Created attachment 21090 [details] [review] Bug 9294 - Move JavaScript out of circulation template This patch creates a new js file, js/pages/circulation.js, and begins the process of moving JavaScript into it from circulation.tt. I have started the process by focusing on sections which are least dependent on template variables. To test, load a patron for checkout who has something checked out. There should be no JavaScript errors: - Tabs should work. - Showing and hiding the "Add message" form should work. - The "specify due date" datepicker should work. - Controls for selecting checkboxes in the checkouts table should work Signed-off-by: Campbell Reid-Tait <campbellreidtait@gmail.com>
Created attachment 21091 [details] [review] Bug 9294 - Move JavaScript out of circulation template This patch creates a new js file, js/pages/circulation.js, and begins the process of moving JavaScript into it from circulation.tt. I have started the process by focusing on sections which are least dependent on template variables. To test, load a patron for checkout who has something checked out. There should be no JavaScript errors: - Tabs should work. - Showing and hiding the "Add message" form should work. - The "specify due date" datepicker should work. - Controls for selecting checkboxes in the checkouts table should work Signed-off-by: Campbell Reid-Tait <campbellreidtait@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Just found a weird behavior with checkboxes in the checkouts table, but still exists on master.
(In reply to Jonathan Druart from comment #19) > Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> > Just found a weird behavior with checkboxes in the checkouts table, but > still exists on master. Arg! It is weird on master, but much more with this patch. With this patch, checkboxes can be all checked together. Failed QA.
Created attachment 21092 [details] [review] Bug 9294: Followup : add missing lines.
Found, 2 lines were missing in the js file. Now we have the same behavior as master.
Pushed to master. Thanks, Owen!
This patch has been pushed to 3.12.x, will be in 3.12.8. It is needed for 10884. Thanks Owen!