[Koha-bugs] [Bug 24201] Attach desk to intranet session

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24201

--- Comment #75 from Josef Moravec <josef.moravec at 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.