Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhance pivot table element to be well formed for potential use with DataTables plugin #369

Closed
wants to merge 1 commit into from

Conversation

astechdev
Copy link

@nicolaskruchten I was hoping to enhance the pivot table html element in order to leverage the DataTables jQuery plugin. However, since the table was missing a tbody element (http://legacy.datatables.net/usage/) the DataTables plugin was throwing an error.

@nicolaskruchten
Copy link
Owner

This is a good idea in principle but the implementation is a bit strange... thetrs containing only ths (i.e. the header rows) should be in the thead element, no?

@astechdev
Copy link
Author

I will send an update later this week...sorry I have been busy at work :)

On Fri, Aug 7, 2015 at 1:31 AM, Nicolas Kruchten [email protected]
wrote:

This is a good idea in principle but the implementation is a bit
strange... thetrs containing only ths (i.e. the header rows) should be in
the thead element, no?


Reply to this email directly or view it on GitHub
#369 (comment)
.

@astechdev astechdev force-pushed the master branch 2 times, most recently from b84f4e7 to 738088c Compare August 12, 2015 04:08
@astechdev
Copy link
Author

@nicolaskruchten I have updated the PR. The main issue with using the DataTables Plugin with your PivotTable Plugin is that you have colspan and rowspan in the tbody tag. I added a datatablesEnabled: false property to the default options and allow the user to override it. If the user overrides then a DataTables Plugin compatible tbody is produced.

To test I built a little app that initializes a table with the PivotTable Plugin
$('#tableId').pivot()
and override
datatablesEnabled: true

Then I enhanced the table with the DataTables Plugin
$('#tableId').DataTables()

The final product is a basic DataTable with sortable columns.
pitch predict pivot table

This should open the door for leveraging any available DataTables features (https://www.datatables.net/manual/api) in the PivotTable

@astechdev astechdev force-pushed the master branch 2 times, most recently from 2ee2cfd to 0abf765 Compare August 27, 2015 16:54
@nicolaskruchten
Copy link
Owner

I'm OK with adding <thead> and <tbody> but I don't think I want DataTables-related code in the core, though. If you really want DataTables-compatible output, I would recommend that you either wrap or fork the TableRenderer

@astechdev
Copy link
Author

Are you referring to the "isDataTables" flag? That flag just removes any
rowspan or colspan from the and I didn't really know a better way
to conditionally remove those...
On Sep 1, 2015 1:38 PM, "Nicolas Kruchten" [email protected] wrote:

I'm OK with adding and but I don't think I want
DataTables-related code in the core, though. If you really want
DataTables-compatible output, I would recommend that you either wrap or
fork the TableRenderer


Reply to this email directly or view it on GitHub
#369 (comment)
.

@nicolaskruchten
Copy link
Owner

Yes, that's what I'm referring to. I don't really like this approach, as the rowspan/colspan is the key feature of this renderer. IMO if we want a renderer that doesn't do this, we should write a new (much simpler) one, rather than adding options to this one.

@astechdev
Copy link
Author

Understood. I may send another pull request for a new renderer then... May
take a bit better I get around to it.
On Sep 2, 2015 9:12 AM, "Nicolas Kruchten" [email protected] wrote:

Yes, that's what I'm referring to. I don't really like this approach, as
the rowspan/colspan is the key feature of this renderer. IMO if we want a
renderer that doesn't do this, we should write a new (much simpler) one,
rather than adding options to this one.


Reply to this email directly or view it on GitHub
#369 (comment)
.

@nicolaskruchten
Copy link
Owner

ok :)

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