The coming patch add the javascript library, a CSS (and image files from datatables.net) and another javascript file which provides some extra features. It also add a Perl module C4::Utils::Datatables which provides helpful functions for server-side processing.
Created attachment 5284 [details] [review] Patch for DataTables
Created attachment 5291 [details] [review] Forgotten patch for translation This patch adds a default option object, which, when passed to dataTable(), set the language strings. Without it, datatables strings can not been translated.
Created attachment 5294 [details] [review] New patch This patch includes the preceding two patches, and fix minor issues (a missing parenthesis in a string, and rename a javascript function). This is the final patch (I hope).
How can this patch be tested?
It can be tested with bug 6838. Apply this patch first and then the patch for 6838 which is an example of datatables usage.
Updating Version : This ENH will be for Koha 3.8
Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday)
(In reply to comment #2) > This patch adds a default option object, which, when passed to dataTable(), set > the language strings. > Without it, datatables strings can not been translated. If you're referring to /intranet-tmpl/prog/en/js/datatables.js, those strings will not be translated because js files are not processed by the translator.
(In reply to comment #8) > (In reply to comment #2) > > This patch adds a default option object, which, when passed to dataTable(), set > > the language strings. > > Without it, datatables strings can not been translated. > > If you're referring to /intranet-tmpl/prog/en/js/datatables.js, those strings > will not be translated because js files are not processed by the translator. So I have to put translatable strings into a .inc file?
(In reply to comment #9) > So I have to put translatable strings into a .inc file? Until we have a fix for Bug 4503, any strings output by JavaScript must be handled in a template somehow. See the Cart for a good example of how that can be done. In includes/doc-head-close.inc: 91 var MSG_NO_RECORD_SELECTED = _("No item was selected"); In js/basket.js 414 if (bibs.length == 0) { showListsUpdate(MSG_NO_RECORD_SELECTED); } 415 return bibs.join("&");
Created attachment 6340 [details] [review] Follow-up changes - Changing default styles for Datatables pager, filter, etc. to make it more like the style we have now on table pagers - Adding another paging style via a plugin function in js/datatables.js to give us a "four_button" option (more like the old tablesorter.js pager). - Tweaking configuration on serials-home.tt and adding the same to readingrec.tt I don't have much serials data so I added it to readingrec.tt as well.
When working on the patch described in Comment 11 I was trying to make the transition from tablesorters.js to Datatables smoother from the user's point of view, providing a compact version of the Datatables UI similar to the tablesorter pager we have now. Now that I've gotten a handle on the basic configuration options I can see that for instances where we use tablesorter.js the transition should be really smooth. I'm a little concerned about the significant increase in the plugin size: About 13K for tablesorter.js (17K if you count the pager plugin too) versus 70K for the datatables base plugin plus 15K for the custom configuration file. But I guess it's a trade-off for the increased functionality... What do you think of the default style changes I'm proposing?
By the way, my patch was created on top of the patch for this bug and the one Bug 6838.
It's much more beautiful like this! But I have some remarks: - Some image files in CSS are not in my local repository and don't come with the patch (img/arrow-*.png). This causes "full_numbers" pagination to have large empty space before or after labels ("First", "Back", etc.) - The first patch provides datatables for opac and intranet, so it would be great to have your work for opac too - The initial goal of this bug is to provide only the javascript library and other stuff (css, images, ...) but no tables using it, so patches contain only new files and could be easy to apply. Maybe you can separate your patch in two parts (and remove the modif on serials-home.tt so we can apply both patches from bug 6836 without applying patch from bug 6838) - If I don't use the 'sDom' property, the display is completely broken, so it has to be fixed - Using the default pagination type, at the bottom we have the two buttons (previous and next) and a right border, immediately followed by the div border (very minor bug) - I prefer images from datatables.net rather than the ones used with tablesorter, but it's just a matter of taste (In reply to comment #12) > I'm a little concerned about the significant increase in the plugin size: About > 13K for tablesorter.js (17K if you count the pager plugin too) versus 70K for > the datatables base plugin plus 15K for the custom configuration file. But I > guess it's a trade-off for the increased functionality... Good point, I haven't noticed that. But it seems that DataTables is still faster than tablesorter. For example, on authorised values admin page, with large list of values (~1000), tablesorter sometimes caused firefox freezes and DataTables don't (but maybe it's just a firefox issue, I don't know) It certainly have to be tested.
Julian, I can confirm that's a Firefox issue. The workaround is to disable the tablesorter entirely. If datatables eliminates the freezing, that seems like a worthwhile exchange. Just set a far-future expiration on the js.
> - Some image files in CSS are not in my local repository and don't come with > the patch (img/arrow-*.png). This causes "full_numbers" pagination to have > large empty space before or after labels ("First", "Back", etc.) Confirmed, sorry about that. > - The initial goal of this bug is to provide only the javascript library and > other stuff (css, images, ...) but no tables using it, so patches contain only > new files and could be easy to apply. Maybe you can separate your patch in two > parts (and remove the modif on serials-home.tt so we can apply both patches > from bug 6836 without applying patch from bug 6838) Sure, I just wanted an easily-testable patch so I could get others' input on the changes I was playing around with. > - If I don't use the 'sDom' property, the display is completely broken, so > it has to be fixed I don't understand. > - I prefer images from datatables.net rather than the ones used with > tablesorter, but it's just a matter of taste Let's get some more opinions. > (In reply to comment #12) > > I'm a little concerned about the significant increase in the plugin size: > > Good point, I haven't noticed that. But it seems that DataTables is still > faster than tablesorter. For example, on authorised values admin page, with > large list of values (~1000), tablesorter sometimes caused firefox freezes and > DataTables don't (but maybe it's just a firefox issue, I don't know) I'm glad you've tested with larger data sets. Performance has certainly been an issue with tablesorter, so it would be nice for that factor to be another plus for DataTables.
Created attachment 6387 [details] Broken display without custom 'sDom' property (In reply to comment #16) > > - If I don't use the 'sDom' property, the display is completely broken, so > > it has to be fixed > > I don't understand. I'm refering to these lines: $("#table_readingrec").dataTable({ "sPaginationType": "four_button", "sDom": '<"top pager"ilpf>t<"bottom pager"ip>' }); If you remove the "sDom" property (so it will take the default value), the display is broken (see screenshot attached).
Created attachment 6467 [details] [review] Signed-off patch adding DataTable jQuert plugin
Created attachment 6468 [details] [review] Signed-off Owen patch styling and using DataTable in reading history I confirm Owen styling is effective both in readingrec.tt (efficent on large reading history) and in serials-home.pl page.
Created attachment 6522 [details] [review] Add jQuery dataTables plugin New patch with translatable strings DataTables strings are now in a separate .inc file
Created attachment 6523 [details] [review] Follow-up CSS work This is the Owen's patch, with "sDom" property added in default datatables options, and modifications for opac too. The readingrec.tt patch is not included in it.
Created attachment 6524 [details] [review] Test case on readingrec.pl
14:38 ~/dev/community_repo/koha (ft/6836|AM u+2)$ git-bz apply 6836 ... Test case on readingrec.pl Apply? [yn] y Applying: Bug 6836: test case readingrec.pl error: patch failed: koha-tmpl/intranet-tmpl/prog/en/modules/members/readingrec.tt:1 error: koha-tmpl/intranet-tmpl/prog/en/modules/members/readingrec.tt: patch does not apply Patch failed at 0001 Bug 6836: test case readingrec.pl When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Patch left in /tmp/Test-case-on-readingrecpl-2_BnTq.patch
Forget it, this patch applies well and better with 3 way merge in git bz. (In reply to comment #23)
The current implementation in readingrec.pl produces the following JS error: k is undefined http://kohadev-staff.campus.foundations.edu/intranet-tmpl/prog/en/lib/jquery/plugins/jquery.dataTables.min.js Line 152 I can't see the cause right off as I'm still reading through the code.
Since there are multiple ways to implement Datatables, it would be nice to see a complete writing describing the method of implementation used in this work including the rational for the chosen method.
I just tested it right now: $ git checkout master $ git pull --rebase $ git checkout -b test6836 $ git bz apply 6836 Bug 6836 - jQuery plugin Datatables integration Add jQuery dataTables plugin Apply? [yn] y Applying: Bug 6836: Add jQuery dataTables plugin Follow-up CSS work Apply? [yn] y Applying: Bug 6836: followup CSS work Test case on readingrec.pl Apply? [yn] y Applying: Bug 6836: test case readingrec.pl Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/readingrec.tt And I get no errors on readingrec.pl. Sorry, but I can't help you :-/
(In reply to comment #26) > Since there are multiple ways to implement Datatables, it would be nice to see > a complete writing describing the method of implementation used in this work > including the rational for the chosen method. You're right, and something like that was already started in BibLibre wiki. So I will do it soon.
(In reply to comment #28) > You're right, and something like that was already started in BibLibre wiki. So > I will do it soon. written, and available here: http://wiki.koha-community.org/wiki/DataTables_HowTo The 1st part (switching actual Tables to datatable) is ready. What is still a WIP is the part related to moving to dynamic loading of the page for large tables. But such a move will have to be made only for a few tables, and it does not prevent this patch to be signed off I also tested this patch, it worked fine, and I couldn't see any javascript error. QA comment: There are 2 minor perlcritic errors: perlcritic C4/Utils/DataTables.pm Code before strictures are enabled at line 25, column 1. See page 429 of PBP. (Severity: 5) Variable declared in conditional statement at line 111, column 5. Declare variables outside of the condition. (Severity: 5) other tests are OK: prove xt/author/valid-templates.t xt/author/valid-templates.t .. ok prove xt/author/translatable-templates.txt/author/translatable-templates.t .. ok
> > > QA comment: > There are 2 minor perlcritic errors: > perlcritic C4/Utils/DataTables.pm > Code before strictures are enabled at line 25, column 1. See page 429 of PBP. > (Severity: 5) > Variable declared in conditional statement at line 111, column 5. Declare > variables outside of the condition. (Severity: 5) > Just FYI severity 5 is the most severe. Those are pretty severe style violations. Chris
(In reply to comment #30) > > There are 2 minor perlcritic errors: > > perlcritic C4/Utils/DataTables.pm > > Code before strictures are enabled at line 25, column 1. See page 429 of PBP. > > (Severity: 5) > > Variable declared in conditional statement at line 111, column 5. Declare > > variables outside of the condition. (Severity: 5) > > > Just FYI severity 5 is the most severe. Those are pretty severe style > violations. Agreed. By "minor" I wanted to say "easy to fix". The 1st is fixed by a use strict at the beginning the 2nd one is also easy to fix by removing the declaration from the condition! but they must be fixed to have the patch pushed
(In reply to comment #31) > The 1st is fixed by a use strict at the beginning Modern::Perl already includes a "use strict". Is it really the way to fix it?
Created attachment 7112 [details] [review] Add jQuery dataTables plugin Corrected patch for perlcritic message: > Variable declared in conditional statement at line 111, column 5. Declare > variables outside of the condition. (Severity: 5) 'use strict' not added since it's redundant with Modern::Perl I've seen that adding these lines to perlcritic.rc prevent this error to be printed: [TestingAndDebugging::RequireUseStrict] equivalent_modules = Modern::Perl [TestingAndDebugging::RequireUseWarnings] equivalent_modules = Modern::Perl
Testing this now, this adds a new dependency on Modern::Perl Which is no big issue, I will do a follow up to add that to the dependencies, and a follow up to change the perlcriticrc I will sign off on these, but I think they still need another sign off, so will leave the status as needs sign off, but they should be in a state to test well now
Created attachment 7120 [details] [review] Bug 6836: Add jQuery dataTables plugin This patch add the javascript library, a CSS (and image files from datatables.net) and another javascript file which provides some extra features. It also add a Perl module C4::Utils::Datatables which provides helpful functions for server-side processing Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 7121 [details] [review] Bug 6836: followup CSS work Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 7122 [details] [review] Bug 6836: test case readingrec.pl Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 7123 [details] [review] Updating perlcriticrc to allow Modern::Perl to suffice instead of use warnings; and use strict;
Created attachment 7124 [details] [review] Bug 6836: Adding dependency on Modern::Perl
Created attachment 7243 [details] [review] [SIGNED-OFF] Updating perlcriticrc to allow Modern::Perl to suffice instead of use warnings; and use strict; Signed off, as perlcritic -p t/perlcriticrc C4/Utils/DataTables.pm returns no error about strictures.
Created attachment 7244 [details] [review] [SIGNED-OFF] Bug 6836: Adding dependency on Modern::Perl
Created attachment 7298 [details] [review] [SIGNED-OFF] Bug 6836: Add jQuery dataTables plugin This patch add the javascript library, a CSS (and image files from datatables.net) and another javascript file which provides some extra features. It also add a Perl module C4::Utils::Datatables which provides helpful functions for server-side processing Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 7299 [details] [review] [SIGNED-OFF] Bug 6836: followup CSS work Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 7300 [details] [review] [SIGNED-OFF] Bug 6836: test case readingrec.pl Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> I tested the functionality and tested translation. All important strings are in the template and can be translated. Good work!
Created attachment 7301 [details] [review] [SIGNED-OFF] Updating perlcriticrc to allow Modern::Perl to suffice instead of use warnings; and use strict; http://bugs.koha-community.org/show_bug.cgi?id=6836 Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 7302 [details] [review] [SIGNED-OFF] Bug 6836: Adding dependency on Modern::Perl Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
QA comments: * no comments to do about the plugin itself, it's external code * strongly tested by many ppl * only a few code outside from the patch itself * documentation added on the wiki good stuff, passed QA !
Ok in master, marked as resolved