https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24201
--- Comment #75 from Josef Moravec <[email protected]> --- (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. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
