Bug 8883

Summary: opacsmallimage doesn't work in ccsr theme
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: OPACAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: major    
Priority: P5 - low CC: chris, eveal, jonathan.druart, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 8883 - Fix for opacsmallimage in ccsr theme.
[SIGNED-OFF] Bug 8883: 'opacsmallimage' doesn't work in ccsr theme
Bug 8883: 'opacsmallimage' doesn't work in ccsr theme

Description Nicole C. Engard 2012-10-05 18:53:21 UTC
In the CCSR theme if you have the opacsmallimage preference set it doesn't do anything. Many libraries use this preference for their own logos and all themes should use it.

Nicole
Comment 1 Nicole C. Engard 2012-10-05 18:54:59 UTC
This might help in finding all the preferences that are supposed to work on the themes: http://manual.koha-community.org/3.10/en/impopac.html#editableopac 

Nicole
Comment 2 Nicole C. Engard 2012-12-19 20:06:13 UTC
Changing assignee to Frédérick since he originally designed the theme and can probably fix this.
Comment 3 Nicole C. Engard 2012-12-26 13:40:15 UTC
Assignee changed per comment on bug 8884.
Comment 4 Martin Renvoize 2012-12-31 13:43:02 UTC Comment hidden (obsolete)
Comment 5 Owen Leonard 2012-12-31 15:15:38 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2012-12-31 15:17:48 UTC
(In reply to comment #4)

> However, it's not a perfect patch. I tried to follow the coding guidelines
> to the letter (found
> http://wiki.koha-community.org/wiki/Coding_Guidelines#HTML1:
> _Template_Toolkit_markup_inside_HTML) but found it difficult to implement
> the patch without breaking Rule 1 in some way.

Your patch doesn't break Rule 1. Rule 1 refers specifically to extended constructions like IF...ELSE inside a tag, not to single variable declarations.
Comment 7 Martin Renvoize 2012-12-31 15:44:04 UTC
Great.. thanks for the clarifications Owen!

Also, happy to switch around the UNLESS/IF's.. I couldn't see any clear motive for doing it either way or indeed a current standard.  Actually, I agree and prefer an IF to an UNLESS, but I threw it together using code initially derived from the prog templates.. which used an UNLESS so it stuck.. ;)
Comment 8 Jonathan Druart 2013-01-15 13:14:52 UTC
QA comment:

This patch allows to use the opacsmallimage pref from the ccsr theme.

Works as expected.

Marked as Passed QA.
Comment 9 Jonathan Druart 2013-01-15 13:15:05 UTC
Created attachment 14599 [details] [review]
Bug 8883: 'opacsmallimage' doesn't work in ccsr theme

This patch makes it possible to customize the CCSR theme using the
opacsmallimage system preference. It also enables the LibraryName
preference for opacsmallimage alt text.

To test, set the opacsmallimage preference and switch to the CCSR
theme. Your custom image should appear. Set the LibraryName preference
and confirm that the alt attribute of the custom logo changes
accordingly.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

This patch works correctly, although it lacked a description and test
plan, which I have added. I would have used IF instead of UNLESS because
to me that seems more readable, but it's not a deal-breaker.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 10 Jared Camins-Esakov 2013-01-16 01:04:31 UTC
This patch has been pushed to master.
Comment 11 Chris Cormack 2013-01-16 05:51:08 UTC
Pushed to 3.10.x will be in 3.10.2