https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24201 --- Comment #75 from Josef Moravec <josef.moravec@gmail.com> --- (In reply to Martin Renvoize from comment #59)
Comment on attachment 107093 [details] [review] Bug 24201: (follow-up) add desk choice with library choice
Review of attachment 107093 [details] [review]: -----------------------------------------------------------------
Signing off the set.. this is a really nice and elegant solution, well done !
Minor note which I'm drawing the QAers attention to, but not a direct failure.. more of a slight code style wonder from my point.
::: koha-tmpl/intranet-tmpl/prog/en/includes/js_includes.inc @@ +25,5 @@
[% Asset.js("lib/jquery/plugins/jquery.validate.min.js") | $raw %] <!-- koha core js --> [% Asset.js("js/staff-global.js") | $raw %] +[% IF setdesk %] +[% Asset.js("js/desk_selection.js") | $raw %]
I'm wondering about the logic here.. could we not just include the Asset line where it's required rather than setting a TT variable to then look for here?
::: koha-tmpl/intranet-tmpl/prog/en/modules/auth.tt @@ +5,4 @@
[% USE Categories %] [% SET footerjs = 1 %] +[% IF Desks.all %] + [% SET setdesk = 1 %]
I think it would be clearer to just include the asset in the JS includes at the bottom of the template here rather than set a variable to be caught to add it in in the js_includes block.
I do agree with you Martin. I rebased the patchset, and added some follow-ups, including removing setdesk variable... Could you please look at my patches? I do not want to pass this QA until somebody else look at my follow-ups. Thanks -- You are receiving this mail because: You are watching all bug changes.