Bug 7594 - Google Cover Javascript contains hardcoded CSS style
Summary: Google Cover Javascript contains hardcoded CSS style
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Owen Leonard
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-24 19:22 UTC by Shane Sammons
Modified: 2016-12-05 21:27 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 7594 - Google Cover Javascript contains hardcoded CSS style (1.80 KB, patch)
2016-03-22 12:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 7594 - Google Cover Javascript contains hardcoded CSS style (1.90 KB, patch)
2016-03-23 08:42 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 7594: Google Cover Javascript contains hardcoded CSS style (1.95 KB, patch)
2016-03-23 14:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Shane Sammons 2012-02-24 19:22:52 UTC
In google-jackets.js there is hard coded "style" on line 48 using...
http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=koha-tmpl/opac-tmpl/prog/en/js/google-jackets.js;h=70b1b84337c4c3807cd8e1997a2fda6cd9563004;hb=HEAD

I suggest this is moved to a .css file, as to allow the end user to impose changes to the style.

Additionally, it would be nice if a unique class can be added to all the image preview types. Currently they are all dynamic (class & id). Added a class such as "gbs-cover" or similar would allow end users to use CSS to uniquely style each book cover used. I am not able to find where this is generated or I would suggest the lines of code to change.

Finally, the "No Image Available" does not appear in the HTML source code. The JS shows it creating a "span" element with a class of no-image. Is there anyway to actually have that appear in the actual source code?

All suggested changes should allow for an end user to have easier and clearer control of OPAC search generated images.
Comment 1 Liz Rea 2012-02-24 19:54:31 UTC
(In reply to comment #0)
> In google-jackets.js there is hard coded "style" on line 48 using...
> http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=koha-tmpl/opac-tmpl/prog/en/js/google-jackets.js;h=70b1b84337c4c3807cd8e1997a2fda6cd9563004;hb=HEAD
> 
> I suggest this is moved to a .css file, as to allow the end user to impose
> changes to the style.

Changes that make the styling of the OPAC more granular are always welcome.

> 
> Additionally, it would be nice if a unique class can be added to all the image
> preview types. Currently they are all dynamic (class & id). Added a class such
> as "gbs-cover" or similar would allow end users to use CSS to uniquely style
> each book cover used. I am not able to find where this is generated or I would
> suggest the lines of code to change.

A good point.

> 
> Finally, the "No Image Available" does not appear in the HTML source code. The
> JS shows it creating a "span" element with a class of no-image. Is there anyway
> to actually have that appear in the actual source code?
> 
> All suggested changes should allow for an end user to have easier and clearer
> control of OPAC search generated images.
Comment 2 Owen Leonard 2012-03-06 21:01:54 UTC
(In reply to comment #0)
> In google-jackets.js there is hard coded "style" on line 48 using...
> http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=koha-tmpl/opac-
> tmpl/prog/en/js/google-jackets.js;h=70b1b84337c4c3807cd8e1997a2fda6cd9563004;
> hb=HEAD

The line in question reads:

$("."+book.bib_key).each(function() {

This is the script's way of looping over elements with that particular class (i.e. any title with that same ISBN.). There is nothing at that line which sets a hard-coded style.

There is an invalid style hard-coded in opac-results.tt at line 542 and 543 (in master):

<div style="block"...

The correct way to say that would be <div style="display:block"... but that is redundant for divs, so I recommend we remove the style attribute.

> Finally, the "No Image Available" does not appear in the HTML source code.

It does in two places. In the template (opac-results.tt) it is output if the record has no ISBN:

[% IF ( SEARCH_RESULT.normalized_isbn ) %]...[% ELSE %]
<span class="no-image">No cover image available</span>[% END %]

Then the various scripts output the same thing if they don't return an image. For instance, in google-jackets.js:

    var message = document.createElement("span");
    $(message).attr("class","no-image");
    $(message).html(NO_GOOGLE_JACKET);
    $(this).append(message);

This creates the <span> element, adds the "no-image" class, fills the tag with the NO_GOOGLE_JACKET message (specified in opac-result.tt) and appends the newly-built <span> to the <div>.

The "no-image" class can be modified globally, but if you want multiple cover sources enabled at the same time and want the messages styled differently you'll have to add another class as well, both in the JS and the template:

    $(message).attr("class","no-image google");

Then your custom stylesheet could address it:

span.no-image.google { border: 3px dotted blue; }

And you could write some JS for OpacUserJS to change the message using that class to target it.
Comment 3 Shane Sammons 2012-03-07 04:07:22 UTC
Yeah, we addressed everything today via IRC...

However, I made a mistake, not line 48, but 58 has the following:

    $(this).append(
  58                              '<div style="margin-bottom:5px; margin-top:-5px;font-size:9px">' +
  59                              '<a '+target+'href="' +
  60                              book.info_url +
  61                              '"><img src="' +
  62                              'http://books.google.com/intl/en/googlebooks/images/gbs_preview_sticker1.gif' +
  63                              '"></a></div>'

We should probably fix that to be a class and put it in the .css file. I will look into what file (hopefully tomorrow).
Comment 5 Owen Leonard 2016-03-22 12:24:37 UTC Comment hidden (obsolete)
Comment 6 Marc Véron 2016-03-23 08:42:42 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2016-03-23 14:42:31 UTC
Created attachment 49440 [details] [review]
Bug 7594: Google Cover Javascript contains hardcoded CSS style

The JavaScript which handles the display of Google book cover images
includes a hard-coded "style" attribute. It may have been relevant to
the design of the old prog template, but it doesn't seem to have any use
in the Bootstrap one. This patch removes it.

I have replaced the style attribute with a class in case someone needs
a hook for some custom CSS.

To test, apply the patch, enable the GoogleJackets system preference,
and clear your browser cache if necessary.

- Search for a title in the OPAC which has a Google book cover image
  associated with it.
- View the detail page for that title. Confirm that the "Google Preview"
  link underneath the cover image looks correct.

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Brendan Gallagher 2016-03-23 23:08:24 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!