|
Description
Matthias Meusburger
2019-06-05 12:30:58 UTC
Created attachment 90337 [details] [review] Bug 23050: Plugin hook to add tabs in intranet biblio details page Test Plan: 1) Enable plugins 2) Download and install the latest version of this plugin https://git.biblibre.com/biblibre/koha-plugin-intranet-detail-hook 3) Browse to catalogue/detail.pl for a record 4) Note you see two new tabs with content Created attachment 90348 [details] [review] Bug 23050: Plugin hook to add tabs in intranet biblio details page Test Plan: 1) Enable plugins 2) Download and install the latest version of this plugin https://git.biblibre.com/biblibre/koha-plugin-intranet-detail-hook 3) Browse to catalogue/detail.pl for a record 4) Note you see two new tabs with content Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de> Hi Julian,
nice patch!
1) Some complaints form the QA script:
FAIL koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
FAIL filters
wrong_html_filter at line 173 ( <li><a href="#[% tab.id %]">[% tab.title %]</a></li>)
missing_filter at line 173 ( <li><a href="#[% tab.id %]">[% tab.title %]</a></li>)
missing_filter at line 173 ( <li><a href="#[% tab.id %]">[% tab.title %]</a></li>)
2) Wondering: What happens if the tab name consists non-latin characters, diacritics etc. Will this still work ok as id?
+ $tab->{id} = $tab->{title};
+ $tab->{id} =~ s/[^\w]+/-/g;
I've found this looking for valid characters in ids:
he value must not contain any space characters. HTML 4: ID and NAME tokens must begin with a letter ([A-Za-z]) and may be followed by any number of letters, digits ([0-9]), hyphens ("-"), underscores ("_"), colons (":"), and periods (".").
3) Can you please document the new hooks on the wiki (once we have this in PQA)?
https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
Created attachment 91523 [details] [review] Bug 23050: Plugin hook to add tabs in intranet biblio details page Test Plan: 1) Enable plugins 2) Download and install the latest version of this plugin https://git.biblibre.com/biblibre/koha-plugin-intranet-detail-hook 3) Browse to catalogue/detail.pl for a record 4) Note you see two new tabs with content Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de> Created attachment 91524 [details] [review] Bug 23050: Add missing template filters Created attachment 91525 [details] [review] Bug 23050: Fix tab's id to avoid compatibility issues MDN says: Using characters except ASCII letters, digits, '_', '-' and '.' may cause compatibility problems, as they weren't allowed in HTML 4. Though this restriction has been lifted in HTML5, an ID should start with a letter for compatibility. https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id All issues mentioned in comment 3 should be fixed. I also modified the https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks page to include the bug number and the first Koha version that includes the hooks. Julian, I think this is a pretty cool idea.
I don't like it that you generate content on the .pl (tab id, title, etc). It feels like all that could be done on the templates if we pass the plugins like:
Why not just:
my @tab_plugins = Koha::Plugins->new()->GetPlugins({
method => 'intranet_catalog_biblio_tab',
});
$template->param(
plugins => \@plugins,
tab_plugins => \@tab_pluginss,
);
Also, (for a later enhancement) there should be a way to define the order in which the tabs will be placed. Not sure how/where.
(In reply to Tomás Cohen Arazi from comment #8) > Julian, I think this is a pretty cool idea. > I don't like it that you generate content on the .pl (tab id, title, etc). > It feels like all that could be done on the templates It could be done in templates, but, as we need the tab id in two different places, it means it would duplicate the code that creates that id, right ? Created attachment 93222 [details] [review] Bug 23050: Fix creation of biblio tab's id (Too much javascript in my head...) Created attachment 95004 [details] [review] Bug 23050: Plugin hook to add tabs in intranet biblio details page Test Plan: 1) Enable plugins 2) Download and install the latest version of this plugin https://git.biblibre.com/biblibre/koha-plugin-intranet-detail-hook 3) Browse to catalogue/detail.pl for a record 4) Note you see two new tabs with content Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 95005 [details] [review] Bug 23050: Add missing template filters Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 95006 [details] [review] Bug 23050: Fix tab's id to avoid compatibility issues MDN says: Using characters except ASCII letters, digits, '_', '-' and '.' may cause compatibility problems, as they weren't allowed in HTML 4. Though this restriction has been lifted in HTML5, an ID should start with a letter for compatibility. https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 95007 [details] [review] Bug 23050: Fix creation of biblio tab's id (Too much javascript in my head...) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 95013 [details] [review] Bug 23050: (QA follow-up) Add Koha::Plugins::Tab class This minimal class encapsulates the tabs to be passed around to the templates, so error checking on missing bits is done in a single place. It throws exceptions on errors Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Thx, Tomas! Nice work! Pushed to master for 19.11.00 Tomás, didn't you forgot to "use Try::Tiny;" in catalogue/detail.pl for your last patch? (attachment 95013 [details] [review]) Without the use statement, I get the following error when trying to display content: Can't locate object method "catch" via package "1" (perhaps you forgot to load "1"?) at /home/koha/src/catalogue/detail.pl line 89. With the use statement, it works properly. (In reply to Matthias Meusburger from comment #18) > Tomás, didn't you forgot to "use Try::Tiny;" in catalogue/detail.pl for your > last patch? (attachment 95013 [details] [review] [review]) > > Without the use statement, I get the following error when trying to display > content: > > Can't locate object method "catch" via package "1" (perhaps you forgot to > load "1"?) at /home/koha/src/catalogue/detail.pl line 89. > > With the use statement, it works properly. I did Created attachment 95406 [details] [review] Bug 23050: (RM follow-up) Add Try::Tiny to controller Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> |