Google Books Preview should not require Dojo or any other framework

Bug #1685840 reported by Dan Scott on 2017-04-24
This bug affects 1 person
Affects Status Importance Assigned to Milestone

Bug Description

* Evergreen 3.0

In solidarity with bugs such as #946110 ("Our aging version of Dojo threatens Chrome support") and #1411699 ("TPAC: Don't load dojo widgets unless we actually need them (for autocomplete)"), we should remove our dependence on Dojo or any other JavaScript library meant to provide cross-browser support, as modern browsers (Firefox, Chrome, IE 11, Edge, Safari, Opera) provide consistent enough DOM and JavaScript support that we can relatively easily provide cross-browser support with vanilla JavaScript.

Loading Dojo (or most other cross-browser libraries) requires many more network requests and parsing and rendering of JavaScript and CSS, which slows down the perceived responsiveness of each page--particularly on machines that our users are likely to use, rather than on the normally high-end phones and computers that developers get to use.

Dan Scott (denials) wrote :

See;a=shortlog;h=refs/heads/user/dbs/lp1685840_dojoless_google_books for a series of patches that:

a) reworks the core JavaScript to require only vanilla JavaScript, and to avoid polluting the global namespace in the process
b) uses EventListeners instead of href=javascript: links
c) avoids loading the Google Books preview JSAPI unless a matching ISBN is found in the Google Books volume API
d) modifies display style directly rather than abstracting it behind hideMe()/unHideMe()
e) uses the viewport's actual dimensions as reported by the browser rather than hardcoding a 600x800 view of the book
f) uses JavaScript to determine the language in which the preview should be displayed rather than having TT2 have to process the script
g) includes the script externally (enabling caching - note that we will want to add this to #1681095 "cache-busting" if that enhancement ends up getting merged) rather than inlining it into each /eg/opac/record page

Dan Scott (denials) wrote :

Local testing looks good on IE11, Safari, Firefox, and Chrome.

Dan Scott (denials) wrote : is an sample record in the wild that should display a Google Books Preview, if it helps testing at all.

tags: added: pullrequest
Ben Shum (bshum) wrote :

Hi Dan, I tested the changes and things mostly look okay, except for one issue... when I first open record details, I can see the icon for Google Books and if I click on that it opens it up under the widget. However, the area itself does not change into a hover link for the arrow or "google preview" text next to it, like the other tabs do (for shelf browser, etc.) Also, on initial page load, scrolling down to the "Google Preview" arrow and trying to click on the arrow or text yield nothing. It isn't until I click on the Google Books preview shortcut above the format icons that things work.

Tested with Firefox, Chrome and Microsoft's Edge browser (Windows 10).

For other people testing, the thing to do is:

1) turn on Google Books preview in /openils/var/templates/opac/parts/config.tt2; set variable ctx.google_books_preview = 1

2) search for an example record. The one in Concerto test dataset is ISBN 4431287752 "Breathing, feeding, and neuroprotection [electronic resource]"

Changed in evergreen:
importance: Undecided → Wishlist
status: New → Triaged
Ben Shum (bshum) wrote :

Granted, perhaps this isn't a big deal if most people will be clicking on the obvious Google books preview button right at the top. But it confused me when I couldn't expand the area at the bottom at first.

tags: added: needsreleasenote
Dan Scott (denials) wrote :

Thanks so much for testing, Ben! Right, I'll add an event listener to the bottom section. Patch (and release note) coming up.

Dan Scott (denials) wrote :

I've added a commit to the branch that restores the expected behaviour to the bottom extras bar.

Ben Shum (bshum) wrote :
tags: added: signedoff
Dan Scott (denials) wrote :

Thanks Ben! I have force-pushed your signed-off branch, and adde a release notes commit, to my branch at;a=shortlog;h=refs/heads/user/dbs/lp1685840_dojoless_google_books

Galen Charlton (gmc) on 2017-05-04
Changed in evergreen:
status: Triaged → Confirmed
Galen Charlton (gmc) on 2017-05-04
Changed in evergreen:
assignee: nobody → Galen Charlton (gmc)
Galen Charlton (gmc) on 2017-05-04
tags: removed: needsreleasenote performance
Galen Charlton (gmc) wrote :

Pushed to master. Thanks, Dan and Ben!

Changed in evergreen:
assignee: Galen Charlton (gmc) → nobody
status: Confirmed → Fix Committed
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers