Skip to content

Conversation

milux
Copy link

@milux milux commented Oct 19, 2014

This patch tries to introduce "advanced rendering" into DataTables.
"Advanced" is meant to express that the (display) render function is not limited to just returning the new content of a table cell, but rather change (render) the table cell directly.
This includes classes, css style and - of course - the content of the cell.

This is realized by using the call() function to execute the render function in the context of the table cell, e.g. "this" refers to the rendered cell.

Example:

Old code to color cells with different css classes depending on their data:

        var fnCreatedPointsCell = function (nTd, sData) {
            //eTd: jQuery element, iPoints: determined level relevant for CSS (limited to 5 via Math.min() => points5plus class)
            var eTd = $(nTd), iPoints = Math.min(parseInt(sData), 5);
            //iterate over classes and add/remove them accordingly
            $.each(["points0", "points1", "points2", "points3", "points4", "points5plus"], function(i, v) {
                if(i === iPoints) {
                    eTd.addClass(v);
                } else if(eTd.hasClass(v)) {
                    eTd.removeClass(v);
                }
            });
        };

used by the respective column with

                render: {
                    display: function(data) {
                        return data;
                    }
                },
                createdCell: fnCreatedPointsCell

Findings:

  • Complicated, rendering can't change the color of the cell directly (needs to be re-created).
  • When modifying cell content with cell.data(), the class/color is therefore not changed. Another dirty fix (not explicitly mentioned here) is required to fix this without recreating the whole table's DOM.

Realizing this functionality with the new render algorithm is very easy and doesn't require any dirty fixes:

        var cellRender = function (data) {
            if(data === undefined) {
                return "-";
            }
            //td: jQuery element, points: determined level relevant for CSS (limited to 5 via Math.min() => points5plus class)
            var td = $(this).text(data), points = Math.min(parseInt(data), 5);
            //iterate over classes and add/remove them accordingly
            $.each(["points0", "points1", "points2", "points3", "points4", "points5plus"], function(i, v) {
                if(i === points) {
                    td.addClass(v);
                } else if(td.hasClass(v)) {
                    td.removeClass(v);
                }
            });
        };

used in the column (note the absence of the "createdCell" callback) with

                render: {
                    display: cellRender
                }

I tried to design this patch as much backwards compatible as possible, using

  • a new table-wide parameter "newCellRender" which has to be set "true" to enable the new rendering
  • code which makes the rending to fill the cell with whatever is returned by the display render function when this is not "undefined"

Additional things:
I had to introduce a new (fifth) render type (besides "display", "sort", "filter" and "type"), called "adjust". This renderer is used for the automatic width calculation.
Since the original implementation just removes markup tags from the return value of the display render function and uses the pure text for width approximation

s = _fnGetCellData( settings, i, colIdx, 'display' )+'';
s = s.replace( __re_html_remove, '' )

it should not be necessary to define the "adjust" renderer in most of the cases.
Compatibility: If "newCellRender" is not "true", "adjust" is not used and the implementation falls back to the old behaviour, using "display" as its source.

@milux
Copy link
Author

milux commented Mar 7, 2015

Hello devs, anybody who wants to review this?
I would really love to see this (or a similar) feature implemented in DataTables!
Regards, milux

Conflicts:
	js/core/core.constructor.js
	js/core/core.data.js
@DataTables
Copy link
Collaborator

Sorry - I thought I had responded to this at the time - apparently not! I'm a little bogged down at the moment with a number of other things, so I can't review at the moment, but I will try to as soon as I can.

Allan

@milux
Copy link
Author

milux commented Apr 21, 2016

Hello Allan,

are you still bogged down?
The problem with change sets like that one is that it gets harder and harder to integrate as the code is changed continuously. It was already a lot of effort to nicely integrate my changes into the various files of DataTablesSrc.
It makes me a bit sad to see that this request is now open for 1.5 years, because meanwhile you've changed a lot of things that make it hard to redo this changes properly.
That said, I have no resources anymore to do such a integration in the recent source files again.

Kind regards

@DataTables
Copy link
Collaborator

Hi - yes I'm really sorry I haven't been able to pull this in yet. However, I've not forgotten about it - when I get the time to move on to the next major version of DataTables I hope to introduce this feature then and I'll be using ideas from here.

@milux
Copy link
Author

milux commented Apr 21, 2016

And do you have any concrete plans when this is going to happen? ;)

@DataTables
Copy link
Collaborator

Nope. I truly wish I did - but the amount of time that support takes up is just phenomenal. It makes planning anything very difficult and thus I'm not giving out any more dates.

@milux
Copy link
Author

milux commented Apr 21, 2016

Understood. ;)
I will stick to my customized version and hope that it won't take another year until an official version arrives with support for this...
One last question: Why do you want to wait for so long? I mean... I took extra efforts to make this virtually-100-percent-compatible, hoping for a quick merge in one of the minor versions. What made this stuff so special that you decided to wait until the arrival of a major release?

@DataTables
Copy link
Collaborator

The lack of unit tests in DataTables, which I am working to resolve now. I hope it won't be a year as well - I'll be very disappointed if it is.

@milux
Copy link
Author

milux commented Apr 21, 2016

That point is true indeed... However, the unit tests were already broken and outdated when a posted this one and a half years ago. (I know because I tried to use them for testing my implementation again. ;))
Since then, you introduced a bunch of changes to DataTables without the safety net of unit tests, why so careful in this particular case? ;)

@DataTables
Copy link
Collaborator

Because I felt it had potentially larger impact.

I realise you are disappointed. So am I. I truly wish I had more time and I very much appreciate your taking the time to send the PR. I am working as much as is possible on all of this! It is proving to be difficult to balance everything.

I can say that I hope to include something along these lines in future. I can't say it will be 100% compatible, but more advanced rendering will be in the next major version.

@milux
Copy link
Author

milux commented Apr 21, 2016

Well, regarding the impact you're potentially right 😆 I wouldn't be so annoying if it wouldn't matter to me.
Anyway, thanks for your patience, I don't want to steal more of your valuable time as you need it to fix your messed-up test suite, right? 😉
Feel free to contact me if you need any explanation or inspiration with the advanced rendering stuff when day X arrives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants