From: Demian K. <dem...@vi...> - 2010-08-18 15:36:48
|
I've had time to play with this a bit this morning. A few more comments: 1.) It might be worth adding the note from the JIRA ticket about Google branding requirements as a comment in config.ini. If we do this and leave the setting commented out by default, that should make the feature fairly easy to find without unintentionally getting anyone in trouble. 2.) Would you mind normalizing your tabs to spaces for consistency with the rest of the codebase? 3.) I notice one or two typos of "Haithi" instead of "Hathi." 4.) I saw at least one instance of English text without an accompanying {translate} tag (the "view online" string for HathiTrust). 5.) When assigning LCCN and OCLC number to the template in IndexRecord::getHoldings, it might be better to prefix the variable names with "holding" to minimize the chances of a clash (unlikely... but I'm paranoid). 6.) Your LCCN normalization looks good to me. If you want a point of comparison, you can take a look at the routine I wrote for SolrMarc here: https://vufind.svn.sourceforge.net/svnroot/vufind/branches/authority/import/index_scripts/getNormalizedLCCN.bsh. Actually, though, yours appears slightly more flexible and rigorous than mine. 7.) It's too bad that the configuration processing code is duplicated between Record/Record.php and Search/Results.php. Would it make sense to factor this somewhere else so it can be more easily maintained and merged with other modules? Maybe web/sys/ConfigArray.php? 8.) I notice that some <img> tags have an invalid width attribute of 70px. It should either be "width='70'" or "style='width: 70px;'". 9.) My HTML validator seems to object to the presence of multiple <a> tags all named " ". Similarly, it complains about some anchors where the id and name contain different values. 10.) I experienced very slow OpenLibrary load times in search results - 2.5 minutes for this URL: http://openlibrary.org/api/books?bibkeys=ISBN0140431136,LCCN78314378,OCLC4057008,OCLC37656526,ISBN0812211359,LCCN82021971,OCLC8954038,OCLC12323662,ISBN0192545086,LCCN50008933,OCLC13574046,OCLC9275546,ISBN0404644562,LCCN2001022564,OCLC46573047,OCLC4822964,ISBN0413744604,LCCN00362872,OCLC43674549,OCLC3181772,OCLC6656563,ISBN0826495265,LCCN2008026928,OCLC233029404,ISBN1405899875,LCCN2008028584,OCLC274794202,LCCN09003446,OCLC3388811,&callback=ProcessOLBookInfo . This is probably beyond our ability to change, but frustrating nonetheless! Given the long load times, would it make sense to show some kind of status graphic while searches are being performed, or is this not possible due to the way the APIs are designed? 11.) I got strange results for a multi-volume title that I tried (https://library.villanova.edu/Find/Record/512562) - different services provided previews of different volumes. Probably difficult to improve, but maybe something to think about. 12.) The HathiTrust API is giving an error message for this search: http://catalog.hathitrust.org/api/volumes/brief/json/id:HT25984;isbn:0140431136;lccn:78314378;oclc:4057008|id:HT601677;oclc:37656526|id:HT22146;isbn:0812211359;lccn:82021971;oclc:8954038|id:HT77080;oclc:12323662|id:HT128452;isbn:0192545086;lccn:50008933;|id:HT319402;oclc:13574046|id:HT466695;oclc:9275546|id:HT586287;isbn:0404644562;lccn:2001022564;oclc:46573047|id:HT147713;oclc:4822964|id:HT569969;isbn:0413744604;lccn:00362872;oclc:43674549|id:HT343173;oclc:3181772|id:HT97786;oclc:6656563|id:HT1094848;isbn:0826495265;lccn:2008026928;oclc:233029404|id:HT1208911;isbn:1405899875;lccn:2008028584;oclc:274794202|id:HT512562;lccn:09003446;oclc:33888&callback=ProcessHTBookInfo . Is there someone we should report this to? In spite of my long list of nitpicks, this looks great, and I would be pretty comfortable with its addition to the trunk once you've made a few of the minor adjustments noted above, especially if it's configured as an "opt in" feature. I'll leave it to your discretion when you feel it is ready! thanks, Demian From: Eoghan Ó Carragáin [mailto:eog...@gm...] Sent: Saturday, August 14, 2010 9:37 AM To: vuf...@li... Subject: Re: [VuFind-Tech] Book previews Hi, I have updated the book previews patch. It now includes: * support for Haithi Trust API; * better LCCN normalisation (feedback appreciated); * Open Library & Haithi Trust graphics stored locally (courtesy of NLA); * getLCCN is now a protected function called by getHoldings() in IndexRecord.php (following Demian's suggestion); * support for mobile theme (this required referencing scripts.js in the mobile layout.tpl - is this a problem?) I have already committed the NLA's graphics to the trunk under web/images. Demian (or anyone else), it would be great if you could test it when you get a chance. All the best, Eoghan On 12 August 2010 06:54, eog...@gm...<mailto:eog...@gm...> <eog...@gm...<mailto:eog...@gm...>> wrote: Great. Thanks! Sent from my iPhone On 12 Aug 2010, at 00:29, Mark Triggs <mt...@nl...<mailto:mt...@nl...>> wrote: > Eoghan Ó Carragáin <eog...@gm...<mailto:eog...@gm...>> writes: > >> Hi Demian, >> Thanks for taking the time to look at this. >> >> 1) The reason I loaded the button from the NLA is because that is >> where I found the graphic and I didn't want to just take it without >> permission. Mark, do you see any problems with the NLA's Open >> Library and Haithi Trust button graphics being included in the >> VuFind trunk? > > I've just checked with the people who designed the graphics, and they're > not worried at all, so go ahead! > > Mark > > -- > Mark Triggs > Systems Administrator > Business Systems Support, The National Library of Australia > <mt...@nl...<mailto:mt...@nl...>> > > ------------------------------------------------------------------------------ > This SF.net email is sponsored by > > Make an app they can't live without > Enter the BlackBerry Developer Challenge > http://p.sf.net/sfu/RIM-dev2dev > _______________________________________________ > Vufind-tech mailing list > Vuf...@li...<mailto:Vuf...@li...> > https://lists.sourceforge.net/lists/listinfo/vufind-tech |