https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12461
--- Comment #31 from Katrin Fischer <[email protected]> --- Comment on attachment 49121 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=49121 Bug 12461 - Add patron clubs feature Review of attachment 49121: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=12461&attachment=49121) ----------------------------------------------------------------- Did a first code review - not a lot of findings and mostly minor. Haven't tested the code yet. Patch doesn't apply atm - not sure why the modules appear in the general syspref.pm? <<<<<<< HEAD use C4::Log; ======= use Koha::Clubs; use Koha::Club::Enrollments; >>>>>>> Bug 12461 - Add patron clubs feature ::: Koha/Template/Plugin/AuthorisedValues.pm @@ +30,4 @@ > > sub Get { > my ( $self, $category, $selected, $opac ) = @_; > + warn "GET( $self, $category, $selected, $opac )"; Unconditional warn - please remove. ::: koha-tmpl/intranet-tmpl/prog/en/modules/clubs/clubs-add-modify.tt @@ +32,5 @@ > +<div class="yui-t7"> > + <div class="yui-main"> > + [% IF stored %] > + <div class="alert"> > + <p>Your club was [% IF stored == 'updated' %] updated [% > ELSE %] saved [% END %]</p> Please avoid constructing sentences like that - it's hard to translate, because grammar and work order is different in other languages. ::: koha-tmpl/intranet-tmpl/prog/en/modules/clubs/clubs.tt @@ +23,5 @@ > + } )); > + }); > + > + function ConfirmDeleteTemplate( id, name, a ) { > + if ( confirm( _("Are you sure you want to delete the club template") > + name + "?" ) ) { It would be nicer for translations to use the new Javascript syntax with placeholders here. @@ +41,5 @@ > + } > + } > + > + function ConfirmDeleteClub( id, name, a ) { > + if ( confirm( _("Are you sure you want to delete the club ") + name > + "?" ) ) { Same as above - placeholders preferrable! ::: koha-tmpl/intranet-tmpl/prog/en/modules/clubs/templates-add-modify.tt @@ +9,5 @@ > +//<![CDATA[ > + > +function CheckForm() { > + if ( !$("#club-template-name").val() ) { > + alert( _("Name is a required field!") ); Would it be possible to use the standard markup for required fields insted here? See administration pages for examples. @@ +30,5 @@ > +<div class="yui-t7"> > + <div class="yui-main"> > + [% IF stored %] > + <div class="alert"> > + <p>Your club template was [% IF stored == 'updated' %] > updated [% ELSE %] saved [% END %]</p> Please don't construct sentences (see above). ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/clubs/enroll.tt @@ +27,5 @@ > + </li> > + [% END %] > + > + <li> > + <a href="#" class="btn" onclick="addEnrollment(); return > false;"><i class="icon-plus"></i> Enroll</a> Switch to FA icon - I saw most of them were already, maybe missed this one? -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] http://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/
