Bug 6836 - jQuery plugin Datatables integration http://wiki.koha-community.org/wiki/DataTables_HowTo
Summary: jQuery plugin Datatables integration http://wiki.koha-community.org/wiki/Data...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P2 enhancement (vote)
Assignee: Julian Maurice
QA Contact: Bugs List
URL:
Keywords:
Depends on: 7119
Blocks: 5345 6838 6842 7167 7169
  Show dependency treegraph
 
Reported: 2011-09-02 10:31 UTC by Julian Maurice
Modified: 2012-10-25 23:10 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Patch for DataTables (191.91 KB, patch)
2011-09-02 10:36 UTC, Julian Maurice
Details | Diff | Splinter Review
Forgotten patch for translation (3.41 KB, patch)
2011-09-02 12:58 UTC, Julian Maurice
Details | Diff | Splinter Review
New patch (193.90 KB, patch)
2011-09-02 14:45 UTC, Julian Maurice
Details | Diff | Splinter Review
Follow-up changes (13.06 KB, patch)
2011-11-18 17:30 UTC, Owen Leonard
Details | Diff | Splinter Review
Broken display without custom 'sDom' property (52.26 KB, image/png)
2011-11-24 08:37 UTC, Julian Maurice
Details
Signed-off patch adding DataTable jQuert plugin (194.05 KB, patch)
2011-11-30 13:46 UTC, Frédéric Demians
Details | Diff | Splinter Review
Signed-off Owen patch styling and using DataTable in reading history (13.29 KB, patch)
2011-11-30 13:48 UTC, Frédéric Demians
Details | Diff | Splinter Review
Add jQuery dataTables plugin (196.86 KB, patch)
2011-12-02 09:55 UTC, Julian Maurice
Details | Diff | Splinter Review
Follow-up CSS work (19.52 KB, patch)
2011-12-02 09:58 UTC, Julian Maurice
Details | Diff | Splinter Review
Test case on readingrec.pl (2.48 KB, patch)
2011-12-02 09:59 UTC, Julian Maurice
Details | Diff | Splinter Review
Add jQuery dataTables plugin (196.88 KB, patch)
2012-01-12 10:34 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6836: Add jQuery dataTables plugin (196.92 KB, patch)
2012-01-13 04:02 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6836: followup CSS work (19.57 KB, patch)
2012-01-13 04:04 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6836: test case readingrec.pl (2.53 KB, patch)
2012-01-13 04:05 UTC, Chris Cormack
Details | Diff | Splinter Review
Updating perlcriticrc to allow Modern::Perl to suffice instead of use warnings; and use strict; (914 bytes, patch)
2012-01-13 04:10 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6836: Adding dependency on Modern::Perl (1.13 KB, patch)
2012-01-13 04:17 UTC, Chris Cormack
Details | Diff | Splinter Review
[SIGNED-OFF] Updating perlcriticrc to allow Modern::Perl to suffice instead of use warnings; and use strict; (994 bytes, patch)
2012-01-19 08:45 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6836: Adding dependency on Modern::Perl (1.21 KB, patch)
2012-01-19 08:46 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6836: Add jQuery dataTables plugin (196.99 KB, patch)
2012-01-23 21:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6836: followup CSS work (19.63 KB, patch)
2012-01-23 21:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6836: test case readingrec.pl (2.72 KB, patch)
2012-01-23 21:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Updating perlcriticrc to allow Modern::Perl to suffice instead of use warnings; and use strict; (1.02 KB, patch)
2012-01-23 21:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6836: Adding dependency on Modern::Perl (1.25 KB, patch)
2012-01-23 21:33 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2011-09-02 10:31:41 UTC
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.
Comment 1 Julian Maurice 2011-09-02 10:36:51 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2011-09-02 12:58:05 UTC Comment hidden (obsolete)
Comment 3 Julian Maurice 2011-09-02 14:45:41 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2011-09-19 20:05:15 UTC
How can this patch be tested?
Comment 5 Julian Maurice 2011-09-20 07:16:14 UTC
It can be tested with bug 6838.
Apply this patch first and then the patch for 6838 which is an example of datatables usage.
Comment 6 Paul Poulain 2011-10-24 11:38:19 UTC
Updating Version : This ENH will be for Koha 3.8
Comment 7 Paul Poulain 2011-10-25 15:05:55 UTC
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)
Comment 8 Owen Leonard 2011-11-17 16:13:51 UTC
(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.
Comment 9 Julian Maurice 2011-11-18 09:21:59 UTC
(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?
Comment 10 Owen Leonard 2011-11-18 13:24:05 UTC
(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("&");
Comment 11 Owen Leonard 2011-11-18 17:30:06 UTC Comment hidden (obsolete)
Comment 12 Owen Leonard 2011-11-18 17:43:20 UTC
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?
Comment 13 Owen Leonard 2011-11-18 17:44:40 UTC
By the way, my patch was created on top of the patch for this bug and the one Bug 6838.
Comment 14 Julian Maurice 2011-11-23 16:06:43 UTC
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.
Comment 15 Jared Camins-Esakov 2011-11-23 16:15:36 UTC
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.
Comment 16 Owen Leonard 2011-11-23 16:24:49 UTC
>   - 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.
Comment 17 Julian Maurice 2011-11-24 08:37:46 UTC Comment hidden (obsolete)
Comment 18 Frédéric Demians 2011-11-30 13:46:11 UTC Comment hidden (obsolete)
Comment 19 Frédéric Demians 2011-11-30 13:48:41 UTC Comment hidden (obsolete)
Comment 20 Julian Maurice 2011-12-02 09:55:56 UTC Comment hidden (obsolete)
Comment 21 Julian Maurice 2011-12-02 09:58:51 UTC Comment hidden (obsolete)
Comment 22 Julian Maurice 2011-12-02 09:59:25 UTC Comment hidden (obsolete)
Comment 23 claire.hernandez@biblibre.com 2011-12-02 13:39:10 UTC
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
Comment 24 claire.hernandez@biblibre.com 2011-12-02 14:37:15 UTC
Forget it, this patch applies well and better with 3 way merge in git bz.

(In reply to comment #23)
Comment 25 Chris Nighswonger 2011-12-02 16:41:15 UTC
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.
Comment 26 Chris Nighswonger 2011-12-02 18:10:08 UTC
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.
Comment 27 Julian Maurice 2011-12-28 15:20:55 UTC
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 :-/
Comment 28 Julian Maurice 2011-12-28 15:25:56 UTC
(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.
Comment 29 Paul Poulain 2011-12-30 10:08:06 UTC
(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
Comment 30 Chris Cormack 2011-12-30 10:17:58 UTC
> 
> 
> 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
Comment 31 Paul Poulain 2011-12-30 10:49:10 UTC
(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
Comment 32 Julian Maurice 2011-12-30 11:05:02 UTC
(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?
Comment 33 Julian Maurice 2012-01-12 10:34:41 UTC Comment hidden (obsolete)
Comment 34 Chris Cormack 2012-01-13 04:01:54 UTC
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
Comment 35 Chris Cormack 2012-01-13 04:02:52 UTC Comment hidden (obsolete)
Comment 36 Chris Cormack 2012-01-13 04:04:18 UTC Comment hidden (obsolete)
Comment 37 Chris Cormack 2012-01-13 04:05:32 UTC Comment hidden (obsolete)
Comment 38 Chris Cormack 2012-01-13 04:10:25 UTC Comment hidden (obsolete)
Comment 39 Chris Cormack 2012-01-13 04:17:22 UTC Comment hidden (obsolete)
Comment 40 Julian Maurice 2012-01-19 08:45:57 UTC Comment hidden (obsolete)
Comment 41 Julian Maurice 2012-01-19 08:46:43 UTC Comment hidden (obsolete)
Comment 42 Katrin Fischer 2012-01-23 21:33:13 UTC
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>
Comment 43 Katrin Fischer 2012-01-23 21:33:25 UTC
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>
Comment 44 Katrin Fischer 2012-01-23 21:33:33 UTC
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!
Comment 45 Katrin Fischer 2012-01-23 21:33:40 UTC
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>
Comment 46 Katrin Fischer 2012-01-23 21:33:47 UTC
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>
Comment 47 Paul Poulain 2012-01-27 10:49:14 UTC
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 !
Comment 48 Julian Maurice 2012-02-22 16:24:58 UTC
Ok in master, marked as resolved