Support for cell click callback #557
Conversation
ghost
mentioned this pull request
|
@nicolaskruchten Is there anything holding you from merging this PR? It'll be wonderful to see this feature as a standard feature. |
|
Thanks for the PR! I'm finally able to take a look at this and I like the approach. I'll try to merge it or something like it over the holidays :) |
| @@ -477,6 +485,9 @@ callWithJQuery ($) -> | |||
| td.className = "pvtVal row#{i} col#{j}" | |||
| td.textContent = aggregator.format(val) | |||
| td.setAttribute("data-value", val) | |||
| #cell click callback | |||
| if opts.clickCallback | |||
| $(td).on("click",makeClickCallback(opts.clickCallback,val,allAttrs,colKey.concat(rowKey),pivotData)) | |||
There was a problem hiding this comment.
so in this function we don't use jQuery, for performance reasons, as there can be many cells.
| filters = {} | ||
| for own key,attr of attributes | ||
| filters[attr] = keys[key] | ||
| return (e) -> cb(e,cellValue,filters,data) |
There was a problem hiding this comment.
not sure about this signature... What are we trying to enable within the callback? Mostly I think people want to be able to extract the matching records, but providing the data in this way doesn't seem like the easiest way to enable that.
There was a problem hiding this comment.
OK, actually with a new method on PivotData this could be really good.
There was a problem hiding this comment.
Hi, sorry for the late answer.
Indeed the best would be to be able to return all matching records for the selected cell, but since this isn't already in memory, it might have really bloated the footprint. I felt it wasn't worth it especially considering that clicking a cell might be a pretty rare use case after all.
So I'm just returning the filters here to let the caller rebuild the data at call time - which isn't all that hard.
|
OK, I've merged 87c2e3d which is based on this PR... thanks for the nudge :) |
I decided to implement a cell click callback before I could see the other PR #530 which seems to take a similar approach.
The may difference is that I'm attaching the event listener directly at cell creation time which I feel is slightly more readable, and I pass the callback in the rendererOptions which has the advantage of not breaking any function signature and I feel is more appropriate.
Of course the main limitation is the same, i.e. it only returns the list of selectors from inside the table, i.e. the row and column keys, but NOT the filters applied outside of it such as the pivotUI filters and inclusions / exclusion API parameters, but I feel this is the best we can do with the current design (I don't want to push individual records into an accumulator like in #141 as the memory would rapidly explode, and it's a lot of computation for a click event that may eventually be quite rare).
No extra test as I'm not really at ease with Jasmine, but all current tests are passing and the example page works fine.