Bug 12162 - Add class="branchcode" to body tag to make OPAC CSS-styleable per branch
Summary: Add class="branchcode" to body tag to make OPAC CSS-styleable per branch
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marc Véron
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 17652
  Show dependency treegraph
 
Reported: 2014-04-29 20:14 UTC by Marc Véron
Modified: 2016-12-02 09:51 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
[Proof of concept] Bug 12162 - Add class="branchcode" to body tag to make OPAC CSS-styleable per branch (3.42 KB, patch)
2014-04-29 20:43 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12162 - Add class="branchcode" to body tag to make OPAC CSS-styleable per branch (4.87 KB, patch)
2014-04-30 09:49 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12162 Add class="branchcode" to body tag to make OPAC CSS-styleable per branch (1.53 KB, patch)
2014-07-30 09:43 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12162 Add class="branchcode" to body tag to make OPAC CSS-styleable per branch (1.10 KB, patch)
2014-08-04 15:54 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12162 Add class="branchcode" to body tag to make OPAC CSS-styleable per branch (1.09 KB, patch)
2014-08-04 15:59 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12162 - Add class="branchcode" to body tag to make OPAC CSS-styleable per branch (2.87 KB, patch)
2014-08-07 23:03 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12162 - Add class="branchcode" to body tag to make OPAC CSS-styleable per branch (2.85 KB, patch)
2014-08-07 23:15 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12162 - Add class="branchcode" to body tag to make OPAC CSS-styleable per branch (35.76 KB, patch)
2014-08-13 19:06 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12162 - Add class="branchcode" to body tag to make OPAC CSS-styleable per branch (35.83 KB, patch)
2014-09-22 07:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 12162 - Add class="branchcode" to body tag to make OPAC CSS-styleable per branch (35.88 KB, patch)
2014-09-22 08:10 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 Marc Véron 2014-04-29 20:14:40 UTC
Libraries with multiple branches should be able to change look&feel of OPAC using branch specific CSS (e.g. in in OPACUserCSS)

This can be done using a branch specific class in the body tag (class="branchcode")

That would be something like
<body id="opac-main" class="CPL">

Using this class, we could style branch specific pages e.g. in OPACUserCSS:

/*Background for all branches*/
body {
background-color: red;
}

/*Yellow background and pink border for branch with code CPL*/
body.CPL {
background-color: yellow;
border: 10px solid pink;
}

/*Green background and red border for branch with code CPL*/
body.FFL {
background-color: green;
border: 10px solid red;
}

I did some tests as a proof of concept. Patch follows.
Comment 1 Marc Véron 2014-04-29 20:43:07 UTC Comment hidden (obsolete)
Comment 2 Liz Rea 2014-04-30 00:37:38 UTC
I like this idea. It also would allow you to use jquery per branch, which I like.
Comment 3 paxed 2014-04-30 07:15:04 UTC
Good idea, but perhaps it should be something like class="branch-FOO", just in case the branch short name FOO would match  another css class.
Comment 4 Marc Véron 2014-04-30 07:56:25 UTC
(In reply to paxed from comment #3)
> Good idea, but perhaps it should be something like class="branch-FOO", just
> in case the branch short name FOO would match  another css class.

Good point!
Comment 5 Marc Véron 2014-04-30 09:49:07 UTC Comment hidden (obsolete)
Comment 6 paxed 2014-05-13 08:48:47 UTC
Should this be "needs signoff"?
Comment 7 Marc Véron 2014-05-13 09:06:52 UTC
(In reply to paxed from comment #6)
> Should this be "needs signoff"?

No, not yet. It's just a proof of concept (woudl appreciate tests, though).

Many files are involved. So I think it would be good get it done in a short timeframe to prevent to much cases of "does not apply". And it should be done for bootstrap theme only, as prog theme is about to be depractated.

Comments are welcome!
Comment 8 Katrin Fischer 2014-05-18 10:58:59 UTC
I really like the idea of this. Additionally to the branchcode it would be nice to have the currently selected language. That would allow to trigger the visibility of text with CSS, while you have to use jQuery to do that right now.

Looking at the code I wonder if we could put the logic into Auth.pm instead, so that the logic doesn't need to be repeated in all the .pl files.
We already do that for some other variables and system preferences.
Comment 9 paxed 2014-05-19 09:02:31 UTC
Perhaps the body-tag should be generated in an include file, so it could be INCLUDEd or PROCESSed - like doc-head-open.inc and doc-head-close.inc are done.
Comment 10 Marc Véron 2014-07-30 07:55:07 UTC
(In reply to Katrin Fischer from comment #8)
> (...) Additionally to the branchcode it would be
> nice to have the currently selected language. That would allow to trigger
> the visibility of text with CSS, while you have to use jQuery to do that
> right now.

We have already a language indicator in the html tag: lang="de-DE"
This makes it possible to youse the :lang selector in your CSS.
Example:
body:lang(de-DE) {background-color: green;}
Comment 11 Katrin Fischer 2014-07-30 07:59:32 UTC
Ah, I didn't know that you could do that with the attribute from the html tag. Thx for the tip!
Comment 12 Marc Véron 2014-07-30 08:13:50 UTC
Comment on attachment 27811 [details] [review]
Bug 12162 - Add class="branchcode" to body tag to make OPAC CSS-styleable per branch

Proof of concept obsoleted. New patch will follow.
Comment 13 Marc Véron 2014-07-30 09:43:10 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2014-08-02 07:29:31 UTC
I like the idea, but it seems to be a problem with translation.
See coding guidelines: HTML1: Template Toolkit markup inside HTML.
When can we override this rule?
Comment 15 Marc Véron 2014-08-02 08:03:14 UTC
Hi Marcel,

Thanks for the comment. I was not aware of the translation restriction.

What do you think about the following:

[% IF ( LoginBranchcode ) %]
<body id="opac-main" class="branch-[%LoginBranchcode%]">
[% ELSE %]
<body id="opac-main" class="branch-default">
[% END %]

Marc
Comment 16 Marcel de Rooy 2014-08-04 07:20:07 UTC
(In reply to Marc Véron from comment #15)
> Hi Marcel,
> 
> Thanks for the comment. I was not aware of the translation restriction.
> 
> What do you think about the following:
> 
> [% IF ( LoginBranchcode ) %]
> <body id="opac-main" class="branch-[%LoginBranchcode%]">
> [% ELSE %]
> <body id="opac-main" class="branch-default">
> [% END %]
> 
> Marc

You still have the branchcode var in a html tag.
Comment 17 Katrin Fischer 2014-08-04 07:39:32 UTC
I'd run a test on this - I think it might be ok. I remember the problem happening with if-else-constructs. But we can test by updating a language and checking if translations will work ok.
Comment 18 Marc Véron 2014-08-04 14:13:23 UTC
The coding guidlines say:
"Avoid [% IF %] inside tags because the translation tools try to read the templates as straight HTML and most TT comments inside tags probably confuse/break them."

I will create a new patch for testing translation with the code from comment #15.
Comment 19 Marc Véron 2014-08-04 15:54:18 UTC Comment hidden (obsolete)
Comment 20 Marc Véron 2014-08-04 15:59:46 UTC Comment hidden (obsolete)
Comment 21 Marc Véron 2014-08-04 17:09:37 UTC
I tested translation with an install of fr-FR.

With fr-FR, I get the same HTML as with en:

<body id="opac-main" class="branch-FFL"> (logged in with branch FFL)
and
<body id="opac-main" class="branch-default"> (not logged in)

The translation seems to work. I would appreciate if somebody could confirm.

Marc
Comment 22 Marcel de Rooy 2014-08-06 11:27:22 UTC
(In reply to Marc Véron from comment #21)
> The translation seems to work. I would appreciate if somebody could confirm.

I tested:
[% IF ( branchFFL ) %]
<body id="opac-main" class="[% branchFFL %]">
[% ELSE %]
<body id="opac-main" class="default">
[% END %]

And confirm that it works (tested under a 3.14.x) when translating.
Although I am not sure if it is the most elegant way to address the problem?
Comment 23 Marc Véron 2014-08-06 12:54:42 UTC
(In reply to M. de Rooy from comment #22)

> Although I am not sure if it is the most elegant way to address the problem?

Hi Marcel,
'Hartstikke bedankt' for testing the translation. I'm glad that it worked.

I'm aware that this solution does not look that elegant. But at the moment I have no idea how to do it better.

Paxed proposed in comment #9 to work with an include similar to koha-tmpl/intranet-tmpl/prog/en/includes/doc-head-open.inc
( BTW:I see there an if-else construct inside a html-tag? )

But the most of the .tt files have an individual ID inside the body tag, like
en/modules/opac-addbybiblionumber.tt:<body id="addtolist" class="popup">
...and it seems, that the ID can not be automatized. Maybe the IDs could be calculated from the file name, but they are not consistent with the file names.
To get all of the files involved I went to koha-tmpl/opac-tmpl/bootstrap and did: 
git grep '<body'

Any hint for an alternative to my solution is welcome :-)
Comment 24 paxed 2014-08-07 08:30:05 UTC
(In reply to Marc Véron from comment #23)
> 
> Paxed proposed in comment #9 to work with an include similar to
> koha-tmpl/intranet-tmpl/prog/en/includes/doc-head-open.inc
> ( BTW:I see there an if-else construct inside a html-tag? )
> 
> But the most of the .tt files have an individual ID inside the body tag, like
> en/modules/opac-addbybiblionumber.tt:<body id="addtolist" class="popup">
> 
> Any hint for an alternative to my solution is welcome :-)

Both PROCESS and INCLUDE allow for parameters, so perhaps something like

[% PROCESS bodytag.inc bodyid="addtolist" %]

and then use the bodyid variable as the id...
Comment 25 Marc Véron 2014-08-07 23:03:28 UTC Comment hidden (obsolete)
Comment 26 Marc Véron 2014-08-07 23:15:06 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2014-08-11 07:18:05 UTC
For some reason, this patch does not pass the QA tools script:

 FAIL   koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt
   OK     forbidden patterns
   OK     tt_valid
   FAIL   valid_template
                bodytag.inc: not found

 FAIL   koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt
   OK     forbidden patterns
   OK     tt_valid
   FAIL   valid_template
                bodytag.inc: not found

My first idea here is that the qa tools script is still looking in prog for the include file instead of bootstrap. So it could be a false negative.
Will add Jonathan to the CC list of this report.
Comment 28 Marc Véron 2014-08-13 12:26:31 UTC
I installed QA tools and found the following in QohA/File/Template.pm, sub check_valid_template:
(...)
 $self->path =~ /opac-tmpl/
      ? 'opac'
      : 'intranet';
    $template_dir =
      File::Spec->rel2abs("koha-tmpl/${tmpl_type}-tmpl/prog/en/modules");
    $include_dir =
      File::Spec->rel2abs("koha-tmpl/${tmpl_type}-tmpl/prog/en/includes"); 
(...)

For OPAC, only prog is tested, not bootstrap.

In this case, it is a false positive...
...and missing includes won't be failed for bootstrap.
Comment 29 Marcel de Rooy 2014-08-13 15:01:25 UTC
(In reply to Marc Véron from comment #28)
> I installed QA tools and found the following in QohA/File/Template.pm, sub
> check_valid_template:
> (...)
>  $self->path =~ /opac-tmpl/
>       ? 'opac'
>       : 'intranet';
>     $template_dir =
>       File::Spec->rel2abs("koha-tmpl/${tmpl_type}-tmpl/prog/en/modules");
>     $include_dir =
>       File::Spec->rel2abs("koha-tmpl/${tmpl_type}-tmpl/prog/en/includes"); 
> (...)
> 
> For OPAC, only prog is tested, not bootstrap.
> 
> In this case, it is a false positive...
> ...and missing includes won't be failed for bootstrap.

Yeah, saw those lines too and copied Jonathan on this report.
Comment 30 Marc Véron 2014-08-13 19:06:51 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2014-08-18 11:10:20 UTC
(In reply to M. de Rooy from comment #27)
> For some reason, this patch does not pass the QA tools script:
> 
>  FAIL   koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt
>    OK     forbidden patterns
>    OK     tt_valid
>    FAIL   valid_template
>                 bodytag.inc: not found
> 
>  FAIL   koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt
>    OK     forbidden patterns
>    OK     tt_valid
>    FAIL   valid_template
>                 bodytag.inc: not found
> 
> My first idea here is that the qa tools script is still looking in prog for
> the include file instead of bootstrap. So it could be a false negative.
> Will add Jonathan to the CC list of this report.

Fixed in the koha-qa repo:
commit c2303b68be33e69198f26cd753172f995ac390eb
Author: Jonathan Druart <jonathan.druart@biblibre.com>
Date:   Mon Aug 18 13:07:18 2014 +0200

    The prog theme for the OPAC is deprecated, now use bootstrap
    
    See bug 12162 comment 27.
Comment 32 Marc Véron 2014-08-19 16:06:58 UTC
Thanks for updating the QA scripts, Jonathan.

The patch passes now with "OK" for all files.
Comment 33 Martin Renvoize 2014-09-22 07:41:38 UTC Comment hidden (obsolete)
Comment 34 Martin Renvoize 2014-09-22 07:43:03 UTC
Works as expected and passes QA scripts. I'm happy this patch adds a useful feature and works as tested.
Comment 35 Jonathan Druart 2014-09-22 08:10:14 UTC
Created attachment 31807 [details] [review]
Bug 12162 - Add class="branchcode" to body tag to make OPAC CSS-styleable per branch

This patch adds a branch sepecific class to all OPAC pages.

Example:
If not logged in, opac-main.pl displays:
<body ID="opac-main" class="branch-default" >
If logged in at branch FFL, it displays:
<body ID="opac-main" class="branch-FFL" >

If you log in, opac-user.pl should display
<body ID="opac-user" class="branch-FFL scrollto" >

To test:
1)
Apply patch.

2)
Add to syspref OPACUserCSS something highly visible, e.g. for branch FFL:
.branch-FFL {
background-color: yellow;
border: 10px solid red;
}

3)
Go to OPAC and login in with a user with home branch FFL

4)
Verify that colors change as appropriate.

5)
Log out. Verify that colors display as before or as defined in class branch-default in OPACUserCSS

6)
Display patch in patch diff view, verify that ids and classes in body tag are consistent with params bodyid and bodyclass in INCLUDE line

7)
Search for regressions

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 36 Tomás Cohen Arazi 2014-09-23 18:50:42 UTC
Patch pushed to master.

Thanks Marc!