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

fix to filter for undefined col #1047

Closed
wants to merge 1 commit into from
Closed

Conversation

trwill
Copy link

@trwill trwill commented Oct 14, 2015

Hello,

First thank you for this project! Big fan and have been using for around 2 years.

I did find an issue, which I've oversimplified in the solution below and was hoping to get some feedback. Basically, if there is a 3 col table with only 2 header cols (ie. ) and filters are enabled, it will print out an extra filter on the 3rd col, with no way to disable (at least that I can find). The sorter treats it as one column, and disables it for sorting (expected result). I would expect filters to be treated the same way as sorting (disabled), but around jquery.tablesorter.js:703 it looks like an extra col is pushed if there is a colspan > 1. So the issue is that the sorter sees two cols and the filter sees three. I've attached a screenshot below to show an example of the issue.

In the for loop in jquery.tablesorter.widgets.js:1033, the call to c.$headerIndexed[ column ] returns undefined (as there is no actual 3rd col), and therefore even if you specify sorter: false in the headers option, it will print anyway.

So anyway, I was wondering your thoughts on this issue. Obviously the solution I've shown below is very oversimplified, and I just wanted to start the discussion before I went and added config options etc. The idea here is that if we are trying to pull data on a col that doesn't exist, set the field to disabled by default.

I think the ultimate solution would maybe be to either:

Set the filter to disabled if $header is undefined, and allow a boolean config option (ignoreColSpan?) to override to the current behavior
Add another array to push colspans to around jquery.tablesorter.js:703 for later access?
If you have a better idea I would be happy to try to implement.

Please be gentle, this is my first pull request to a public project!

  • EDIT: added this on a branch instead of master

tablesorter_extra_filter

@Mottie
Copy link
Owner

Mottie commented Oct 14, 2015

Hi @trwill!

Sorry, I didn't respond in the other pull request you just closed. You're right, I should add an internal check to make sure a column exists before adding a filter for it. The problem though would be that the filter row will only show two columns and not include a colspan... I'll have to think about it a bit. Maybe it'll have to detect the colspan and add it to the filter row as needed.

In a future release, I do plan on adding a filter_customVal option which will allow you to create a custom filter row which would completely avoid this issue; sadly, I haven't had the time, or really the motivation, to work out all of the bugs.

@trwill
Copy link
Author

trwill commented Oct 14, 2015

Haha yeah I knew it really was a complicated issue - I guess my solution was based on the fact that since you are adding the colspan to the columns var, then treat the filter accordingly and print a 3rd disabled.

My initial thought was something like you mentioned - somehow detecting the colspan and have the filter span accordingly. But then what do you do for filtering? Filter both cols?

Idk - I would be happy to help though. For a filter_customVal, are you talking about creating a row independent of the cols that you could specify?

@Mottie
Copy link
Owner

Mottie commented Oct 14, 2015

Yes, the widget will detect if any inputs are in the thead, with the set class name, and build a row if nothing is found.

@Mottie
Copy link
Owner

Mottie commented Oct 17, 2015

Well, I added very basic support for a table when an entire column is set with colspan... I have not thoroughly tested it with all widgets, but so far it is working well with the filter widget (demo).

Please test this update (both the core plugin and filter widget) with your data set and please provide me some feedback on how it performs.

@trwill
Copy link
Author

trwill commented Oct 20, 2015

Thanks @Mottie - this worked great for me, although my data set has one col in <thead> and 2 cols in <tbody>. However, the result now is as expected - when disabling the filter, it disables for both cols and does not attach a filter box under the third col. I forked your fiddle here to demonstrate a similar data set.

Thank you!

Mottie added a commit that referenced this pull request Oct 28, 2015
And prevent error if config.sortVars is undefined

See #746 & #1047
@Mottie
Copy link
Owner

Mottie commented Oct 31, 2015

New version now available. Thanks again!

@Mottie Mottie closed this Oct 31, 2015
@trwill
Copy link
Author

trwill commented Nov 2, 2015

Thank you!!

@Mottie Mottie removed the Next Update label Nov 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants