http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7567 --- Comment #69 from M. Tompsett <mtompset@hotmail.com> --- (In reply to Jonathan Druart from comment #68)
+ $template->param( $lang => 1 ) if $lang; Why ? Seems not used in template. But maybe I am wrong.
Because I was thinking it may exist in an earlier version of the template, and was hoping that this could get backported, not just into master. I didn't search earlier versions, though I think you are correct about it not being necessary in master. I'll add a patch for that.
2) - # $query.= "LIMIT 0, " . $limit; + # $query.= 'LIMIT 0, ' . $limit; Could be removed
However, the parameter $limit was not removed, and so I'd like to leave it to show how it was used. It may come in handy if some libraries go news-happy, and we need to add a system preference to specify the limit on the amount of news displayed.
3) Maybe the following is more readable:
$query .= " WHERE 1 "; if ( $lang ) { $query .= "AND (opac_news.lang='' OR opac_news.lang=?)"; push @values, $lang }
and same for $branchcode
Agreed.
4) s/branch/library in templates
5) in opac-main.tt - [% IF ( koha_news_count ) %] <div id="news" class="container"> + [% IF ( koha_news_count ) %]
Why? It will produce an empty div.
Empty divs can still be styled without generating errors in javascript. And empty divs should not cause massive screen variation over the existing output.
Some QA issues (minor): FAIL koha-tmpl/intranet-tmpl/prog/en/modules/tools/koha-news.tt FAIL forbidden patterns forbidden pattern: tab char (line 233) forbidden pattern: tab char (line 169) forbidden pattern: tab char (line 227) forbidden pattern: tab char (line 229) forbidden pattern: tab char (line 228) forbidden pattern: tab char (line 225) forbidden pattern: tab char (line 167) forbidden pattern: tab char (line 231)
FAIL koha-tmpl/opac-tmpl/prog/en/modules/opac-main.tt FAIL forbidden patterns forbidden pattern: tab char (line 25)
Oh fiddlesticks! Patching.
7) It would be great to pass a hashref to get_opac_news
I don't see the number of parameters on get_opac_news growing past 3. Once you hit that fourth parameter, that seems like the point in time to consider making the parameter a hashref. Hope this addresses your concerns. I'll have a revision up shortly. -- You are receiving this mail because: You are watching all bug changes.