Bug 2889 - templates should not use [% IF ( loop.odd ) %]
Summary: templates should not use [% IF ( loop.odd ) %]
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Owen Leonard
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-12-31 08:18 UTC by Galen Charlton
Modified: 2016-02-26 17:15 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Patch for serials-home.tmpl (4.64 KB, patch)
2009-07-23 03:50 UTC, Chris Cormack
Details | Diff | Splinter Review
Patch for serials acqui-search-result (2.12 KB, patch)
2009-07-23 03:55 UTC, Chris Cormack
Details | Diff | Splinter Review
Patch for dictionary.tmpl (1.97 KB, patch)
2009-10-13 00:23 UTC, Chris Cormack
Details | Diff | Splinter Review
Patch for boraccount.pl (6.22 KB, patch)
2010-03-26 16:53 UTC, Chris Cormack
Details | Diff | Splinter Review
Patch for serials-collection.tmpl (2.59 KB, patch)
2010-05-22 17:31 UTC, Garry Collum
Details | Diff | Splinter Review
Proposed patch for report tmpl's. (12.89 KB, patch)
2010-07-03 21:09 UTC, Garry Collum
Details | Diff | Splinter Review
Proposed patch for guided reports. (1.77 KB, patch)
2010-07-03 21:58 UTC, Garry Collum
Details | Diff | Splinter Review
Patch updates template acqui/aqbasketsearch.tt (1.35 KB, patch)
2014-07-25 04:14 UTC, Karl Menzies
Details | Diff | Splinter Review
Bug 2889 - templates should use [% IF ( loop.odd ) %] - Administration (21.31 KB, patch)
2014-08-14 14:44 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 2889 - templates should use [% IF ( loop.odd ) %] - Administration (20.39 KB, patch)
2014-09-23 12:33 UTC, Owen Leonard
Details | Diff | Splinter Review
BUG: 2889 template aqbasketusersearch add loop.odd (1.40 KB, patch)
2014-09-24 18:53 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 2889 - templates should use [% IF ( loop.odd ) %] - Administration (20.40 KB, patch)
2014-09-24 18:53 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 2889: template aqbasketusersearch add loop.odd (1.47 KB, patch)
2014-09-25 09:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 2889 - templates should use [% IF ( loop.odd ) %] - Administration (20.42 KB, patch)
2014-09-25 09:58 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 00:57:37 UTC


---- Reported by gmcharlt@gmail.com 2008-12-31 08:18:29 ----

When styling a table to use different shading for alternating rows, templates should test the H::T::Pro loop context variable __odd__ instead of checking the value of a toggle variable set by a script.  For example:

<!-- TMPL_UNLESS NAME="__odd__" --><tr class="highlight"><!-- TMPL_ELSE --><tr><!-- /TMPL_UNLESS -->

instead of 

 <!-- TMPL_IF NAME="toggle" --><tr class="highlight"><!-- TMPL_ELSE --><tr><!-- /TMPL_IF -->

To make this cleanup for a given script, both the script and the template should be modified.  As of 2008-12-31, affected scripts are:

acqui/spent.pl
acqui/booksellers.pl
acqui/parcel.pl
admin/authorised_values.pl
admin/aqbudget.pl
admin/branches.pl
admin/marc_subfields_structure.pl
admin/authtypes.pl
admin/auth_subfields_structure.pl
admin/currency.pl
admin/koha2marclinks.pl
admin/letter.pl
admin/itemtypes.pl
admin/cities.pl
admin/aqbookfund.pl
admin/auth_tag_structure.pl
admin/stopwords.pl
admin/smart-rules.pl
admin/marctagstructure.pl
admin/z3950servers.pl
admin/roadtype.pl
admin/categorie.pl
admin/biblio_framework.pl
C4/SIP/ILS/Patron.pm
cataloguing/addbooks.pl
cataloguing/z3950_search.pl
labels/pcard-member-search.pl
members/boraccount.pl
members/moremember.pl
members/member.pl
opac/opac-user.pl
reports/reservereport.pl
serials/distributedto.pl
serials/acqui-search-result.pl
tools/overduerules.pl



---- Additional Comments From joe.atzberger@liblime.com 2009-02-05 11:42:47 ----

Note, one could also implement this with jquery:

$("tr:even").css("background-color", "#bbbbff");

In fact, that's the textbook example:
http://docs.jquery.com/Selectors/even



---- Additional Comments From oleonard@myacpl.org 2009-02-05 12:28:16 ----

It would be nice to have a solution that was a one-off, but I'd much rather modify the template and avoid the processing burden like we see with the jquery table-sorter.



---- Additional Comments From mjr@ttllp.co.uk 2009-06-12 17:22:17 ----

patches appearing on list from Garry Collum <gcollum@gmail.com> - is this bug his now?  Anyone co-ordinating the effort?




---- Additional Comments From joe.atzberger@liblime.com 2009-06-12 17:51:13 ----

I think Gary can have the bug if he wants it.  



---- Additional Comments From gcollum@gmail.com 2009-07-04 23:00:40 ----

Multiple patches submitted.

Toggle variables still appear in these files.

C4/Search.pm
C4/Serials.pm
C4/Acquisition.pm
C4/SIP/ILS/Patron.pm

koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tmpl
koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tmpl
koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-home.tmpl
koha-tmpl/intranet-tmpl/prog/en/modules/z3950/searchresult.tmpl

members/boraccount.pl
opac/opac-user.pl
reports/reservereport.pl
serials/acqui-search-result.pl
serials/serials-home.pl





---- Additional Comments From gcollum@gmail.com 2009-07-23 03:50:09 ----

Created an attachment
Patch for serials-home.tmpl





---- Additional Comments From gcollum@gmail.com 2009-07-23 03:55:33 ----

Created an attachment
Patch for serials acqui-search-result

The listed files in serials are done.  subscription-add.tmpl and serials-home.pl have no toggle variables, just the word toggle in remarks.



---- Additional Comments From gcollum@gmail.com 2009-10-13 00:23:51 ----

Created an attachment
Patch for dictionary.tmpl





---- Additional Comments From colin.campbell@ptfs-europe.com 2010-03-26 16:53:37 ----

Created an attachment
Patch for boraccount.pl





--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 00:57 UTC  ---

This bug was previously known as _bug_ 2889 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=2889
Imported an attachment (id=911)
Imported an attachment (id=912)
Imported an attachment (id=913)
Imported an attachment (id=914)

Actual time not defined. Setting to 0.0
CC member joe.atzberger@liblime.com does not have an account here
CC member mjr@ttllp.co.uk does not have an account here
The original submitter of attachment 911 [details] [review] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.
The original submitter of attachment 912 [details] [review] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.
The original submitter of attachment 913 [details] [review] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.
The original submitter of attachment 914 [details] [review] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.

Comment 1 Garry Collum 2010-05-22 17:31:27 UTC Comment hidden (obsolete)
Comment 2 Galen Charlton 2010-05-24 12:00:46 UTC
Pushed  serials-collection.tmpl patch.
Comment 3 Garry Collum 2010-07-03 21:09:43 UTC Comment hidden (obsolete)
Comment 4 Garry Collum 2010-07-03 21:58:03 UTC Comment hidden (obsolete)
Comment 5 Owen Leonard 2012-02-08 14:13:33 UTC
Instead of marking this invalid shouldn't we re-word it to apply to Template::Toolkit?
Comment 6 Kyle M Hall 2012-02-08 14:49:30 UTC
(In reply to comment #5)

I suppose so. We could do something like this: http://www.robertprice.co.uk/robblog/archive/2004/9/Alternating_Table_Rows_With_Template_Toolkit_And_CSS.shtml

Or even better, something like Tablecloth ( http://cssglobe.com/lab/tablecloth/ ), so we could just add a css attribute to any table that needs striping and have it handled automatically.

> Instead of marking this invalid shouldn't we re-word it to apply to
> Template::Toolkit?
Comment 7 Katrin Fischer 2012-04-03 06:11:49 UTC
Hi Owen, 
should we perhaps move this to a separate bug or include a best practice for 'table striping' on a page in the wiki? I am not sure how this is done right now.
Comment 8 Katrin Fischer 2013-12-05 23:23:02 UTC
Owen, could you take a look and check if this bug is still valid?
Comment 9 Owen Leonard 2014-07-11 19:02:28 UTC
This bug is still valid. I see many instances with something like this:

    [% IF ( user.toggle ) %]
      <tr>
    [% ELSE %]
      <tr class="highlight">
    [% END %]

These can be converted to use [% IF ( loop.odd ) %] (which is already used in many places in the templates) or eliminated because the table is being manipulated by the DataTables plugin.
Comment 10 Karl Menzies 2014-07-25 04:14:29 UTC Comment hidden (obsolete)
Comment 11 Owen Leonard 2014-08-14 14:44:31 UTC Comment hidden (obsolete)
Comment 12 Nick Clemens 2014-09-19 20:03:04 UTC
Applying: BUG: 2889 template aqbasketusersearch add loop.odd
Applying: Bug 2889 - templates should use [% IF ( loop.odd ) %] - Administration
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/z3950servers.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/z3950servers.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/currency.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/currency.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/branch_transfer_limits.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/auth_subfields_structure.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudgetperiods.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/css/staff-global.css
Failed to merge in the changes.
Patch failed at 0001 Bug 2889 - templates should use [% IF ( loop.odd ) %] - Administration
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-2889---templates-should-use--IF--loopodd-----A-zUC7yD.patch
nick@debian:~/koha$
Comment 13 Owen Leonard 2014-09-23 12:33:05 UTC Comment hidden (obsolete)
Comment 14 Nick Clemens 2014-09-24 01:26:34 UTC
Hi Owen,

These are the screens where the alternate highlighting did not work for me:
Authority types
Authority types -> MARC structure -> subfields
Administration -> MARC bibliographic framework
Administration -> MARC bibliographic framework -> MARC structure -> Subfields
Administration -> Classification sources
Administration -> Circulation and fines rules (Doesn't work in circ table but does in all below)
Administration -> Koha to MARC mapping

I double checked on master and they were working before the patch.
Comment 15 Owen Leonard 2014-09-24 17:41:52 UTC
(In reply to Nick Clemens from comment #14)
> These are the screens where the alternate highlighting did not work for me

Did you clear your browser cache? The patch updates the staff client CSS.
Comment 16 sandboxes@biblibre.com 2014-09-24 18:52:23 UTC
Patch tested with a sandbox, by Nick Clemens <nick@quecheelibrary.org>
Comment 17 sandboxes@biblibre.com 2014-09-24 18:53:00 UTC Comment hidden (obsolete)
Comment 18 sandboxes@biblibre.com 2014-09-24 18:53:03 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2014-09-25 09:58:08 UTC
Created attachment 31866 [details] [review]
Bug 2889: template aqbasketusersearch add loop.odd

To Test
1/ In Acquisitions > Create a test Budget
2/ In the Budget > Create at least three Test Funds
3/ Create at least three Users who use the fund
4/ Select Search for Basket User
5/ In the Table results list see the list of users and see the alternate table highlighting

6/ Apply the patch
7/ Refresh the Search for Basket User results window
8/ In the Table results list see the list of users and see the alternate table highlighting in place
9/ Bug fixed

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 20 Jonathan Druart 2014-09-25 09:58:13 UTC
Created attachment 31867 [details] [review]
Bug 2889 - templates should use [% IF ( loop.odd ) %] - Administration

Some scripts pass a template variable to facilitate an alternate table
row class for styling. Other use the 'IF (loop.odd)' construction. I
think we're at the point where the CSS3 :nth-child() selector is
widely-supported enough that we can do without template-based solutions:

http://caniuse.com/#feat=css-sel3

This patch adds such a selector to the staff client CSS and removes the
corresponding template markup from Administration pages. The last in
this series of patches will remove the redundant CSS.

Also in this patch: a few minor markup corrections.

To test, apply the patch and clear your browser cache if necessary. View
the following pages and confirm that alternate table row highlighting
works as before:

 - Administration -> Budgets
 - Acquisitions   -> Vendor -> Contracts
 - Administration -> Budgets -> Budget -> Planning
 - Administration -> Authority types
 - Administration -> Authority types -> MARC structure
 - Administration -> Authority types -> MARC structure -> subfields
 - Administration -> MARC bibliographic framework
 - Administration -> MARC bibliographic framework -> MARC structure
 - Administration -> MARC bibliographic framework -> MARC structure -> Subfields
 - Administration -> Libraries and groups
 - Administration -> Cities and towns
 - Administration -> Classification sources
 - Administration -> Circulation and fines rules
 - Administration -> Currencies and exchange rates
 - Administration -> Item types
 - Administration -> Koha to MARC mapping
 - Administration -> System preferences -> Local use
 - Administration -> Z39.50 client targets

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 21 Jonathan Druart 2014-09-25 09:59:58 UTC
IMO last commit message should be "templates should *not* use".
Comment 22 Jonathan Druart 2014-09-25 10:02:40 UTC
Arg, I didn't update the status deliberately.
Comment 23 Tomás Cohen Arazi 2014-10-15 21:29:49 UTC
Patches pushed to master.

Thanks Karl and Owen!
Comment 24 Fridolin SOMERS 2014-10-22 16:00:41 UTC
We should also remove [% IF ( loop.even ) %] right ?