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

Add "allow_blank" option (default is "true", but it should be "false" IMHO) #112

Closed
wants to merge 4 commits into from

Conversation

Oliboy50
Copy link

I needed this for my project, because it was completely useless to have a blank entry in my filter inputs.

@vedmack
Copy link
Owner

vedmack commented Nov 25, 2014

Looks very detailed :) , bummer you haven't done your modification on more recent version of yadcf (lab version - https://github.com/vedmack/yadcf/blob/master/lab/jquery.dataTables.yadcf.js)

Will inspect it later on...

@Oliboy50
Copy link
Author

Oh, I didn't see this folder. Maybe the "select blank value issue" is already fixed there.

If it's not the case, this PR is as simple as checking if the column data is empty before adding it to the select input ( https://github.com/vedmack/yadcf/pull/112/files#diff-9ef1d6803d7184632df46a815b39de1eR1377 ).

And I'm sure that it would be better if it was the default behavior.
I don't think someone would need a blank value as filter.

Thanks for the plugin!

@vedmack
Copy link
Owner

vedmack commented Nov 25, 2014

Actually you are right, such feature better be a default behavior and it shouldn't even be an option to config, so if you want, you can open a new PR on the lab version for it (no documentation/configuration needed) , if not I guess I will add it myself.

@vedmack
Copy link
Owner

vedmack commented Nov 26, 2014

Shame to close this PR (appreciate the effort), but you are probably right, no need for empty values in the filter at all, so this PR is redundant, feel free to send PR for "hard coded" fix for this issue (on lab version js)

@vedmack vedmack closed this Nov 26, 2014
vedmack added a commit that referenced this pull request Dec 5, 2014
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