We currently sparsely use this plugin on the checkouts table. It would be nice to switch to the equivalent plugin by the datatables authors and bundle it into our package. Doing so would allow other developers to take advantage of the feature as that version is well documented and supported.
Created attachment 92443 [details] [review] Bug 23493: Replace rowGrouping with rowGroup This is an initial attempt to get some feedback.
This is a somewhat rough and ready attempt at replacement to get someone started. I've directly included the new plugin via a CDN link, this would clearly need to be bundled in our datatables bundle, but I didn't want to add all there here in the first instance and confuse reviewers. I'm sure there's still more code that could be removed from the js file but it would be good to get some initial feedback or perhaps for someone to take it on.
Is this patch complete? I'm not seeing the CDN import line among other things :) (In reply to Martin Renvoize from comment #2) > This is a somewhat rough and ready attempt at replacement to get someone > started. > > I've directly included the new plugin via a CDN link, this would clearly > need to be bundled in our datatables bundle, but I didn't want to add all > there here in the first instance and confuse reviewers. > > I'm sure there's still more code that could be removed from the js file but > it would be good to get some initial feedback or perhaps for someone to take > it on.
Created attachment 92444 [details] [review] Bug 23493: Replace rowGrouping with rowGroup This is an initial attempt to get some feedback.
Oops, forgot to squash before uploading to BZ... should be better now.
Created attachment 97324 [details] [review] Bug 23493: Replace rowGrouping with rowGroup This is an initial attempt to get some feedback. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 97344 [details] [review] Bug 23493: Replace rowGrouping with rowGroup This is an initial attempt to get some feedback. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Looks great to me! I also find the styling to be a bit improved.
I think this should be signed off - changing status.
Created attachment 97694 [details] [review] Bug 23493: Replace rowGrouping with rowGroup This patch replaces the unmaintained third party rowGrouping datatables plugin with the supported core rowGroup replacement. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Simple rebase performed.
I am expecting this bug report to add the rowGroup plugin, not bug 23355. It seems easy to make the dependency the other way around.
Created attachment 97704 [details] [review] Bug 23493: Replace rowGrouping with rowGroup This patch replaces the unmaintained third party rowGrouping datatables plugin with the supported core rowGroup replacement. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Dependency re-ordered as requested and suitable re-base undertaken.
609 "fnInitComplete": function(oSettings, json) { 610 // Disable rowGroup plugin after first use 611 // so any sorting on the table doesn't use it 612 //var oSettings = issuesTable.fnSettings(); Should we remove those 3 commented lines?
One change I noticed: when the column are sorted the "grouped tr" are displayed with this new version: https://snipboard.io/HzXJ4l.jpg (before) https://snipboard.io/zRb4sk.jpg (after)
Hmm, Removing the commented lines seems sensible.. I'll do that now.. As for the display discrepancy you mention.. I can't seem to replicate that. https://snipboard.io/1DOacC.jpg (before) https://snipboard.io/gOLAkD.jpg (after) There are subtle styling differences (which I'm sure can be resolved at the CSS level if we want to do so?), but I can't replicate the 'before' you show without grouping or the after with two "Today's" groupings.
I bet you did not sort the column by "due date". Take a look at the "checked out on" and "due date" column on my screenshots.
Created attachment 98041 [details] [review] Bug 23493: Replace rowGrouping with rowGroup This patch replaces the unmaintained third party rowGrouping datatables plugin with the supported core rowGroup replacement. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 98042 [details] [review] Bug 23493: (follow-up) Switch to aDataSort for multi-column sorting This patch adds aDataSort configurations for each sortable column such that we always maintain the group sorting primarily and apply a secondary sort using the column in question.
OK, I see what you mean now.. good catch. See followup which I believe addresses the issue.
I do not think it's correct, now we cannot really sort by due date: https://snipboard.io/TRx43j.jpg
Created attachment 98055 [details] [review] Bug 23493: Replace rowGrouping with rowGroup This patch replaces the unmaintained third party rowGrouping datatables plugin with the supported core rowGroup replacement. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 98056 [details] [review] Bug 23493: Remove commented lines Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Ok so I confused Martin with my previous comments. I wanted to highlight the fact that there was a behavior's change, but did not consider it a regression or blocker. I am happy with how it works as it. I have attached a follow-up to remove the 3 commented lines. Marked as Passed QA!
Nice work everyone! Pushed to master for 20.05
(In reply to Martin Renvoize from comment #26) > Nice work everyone! > > Pushed to master for 20.05 There is something very wrong here Martin. You pushed: Bug 23493: (follow-up) Disable rowGroup on due date sort Bug 23493: (follow-up) Switch to aDataSort for multi-column sorting Bug 23493: Replace rowGrouping with rowGroup They all have my signed off. However I disagreed with the last 2. Additionally the last one has never been published here.
Corrected by reverting the errantly pushed patches and applying the followup here instead.
Created attachment 98118 [details] [review] Bug 23493: Fix selenium/regressions.t One test wants to make sure thead, tfood and tbody will have th/td's with the same size/number. Existing test for tbody was retrieving the grouped td, which has a colspan of 100%. Note that this 100% value seems to be wrong as it is interpreted like "100" by the browser. We do not want to test this row anyway, but the real row, the one with all the td's. This patch makes it retrieve the second tr and count the number of td it has.
Created attachment 98132 [details] [review] Bug 23493: (RM follow-up) Let datatables calculate colspan for us Prior to this we arbitrarily set a colspan of 100 for the rowgroup header row. If we remove the td wrapper from the render function then datatables will automatically wrap the output in an appropraite td with cell with fullwidth colspan correctly set. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This last patch has been pushed to master for 20.05 on Jan 30.