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

Added support for search, sorting, numbers, localization #264

Closed
wants to merge 1 commit into from
Closed

Added support for search, sorting, numbers, localization #264

wants to merge 1 commit into from

Conversation

mpryvkin
Copy link
Contributor

@DataTables
Copy link
Collaborator

Thanks for this! I'll try to make some time later this week to review it :-)

@DataTables
Copy link
Collaborator

Highlevel points:

  1. I don't understand the sorting aspect. This plug-in is a filter, so I don't see either why, or where, sorting would come into play?
  2. To my mind a plug-in such as this should provide one feature only - in this case filtering using the alphabet input. Row grouping show not be provided by this plug-in. In the medium term I plan to release a new row grouping extension for DataTables which would conflict with this for example.
  3. White space has been changed from tabs to 3 spaces. The original formatting should be preserved please :-).
  4. I think the code size could be compressed and made more readable by storing sub-objects into variables for local use. For example properties of settings.alphabetSearch are used a number of times in the constructor - the code would be faster, smaller and more readable if it was stored in a alphaSettings variable or similar.

@mpryvkin
Copy link
Contributor Author

Thanks for your feedback.

Sorry for replacing tabs with spaces, I noticed that only after committing the code. It has been fixed.

You have a special category (feature) for a reason for such plug-ins among searching, ordering, etc. This means to me that these plugins could provide more than just searching, ordering. I think row grouping is necessary for this kind of feature (see https://news.google.com/newspapers, for example)

Also you have some extensions conflicting with others, so it could be up to the user to decide which ones to use.

So ordering is just another functionality provided by this feature plug-in, it sorts rows within its own row group.

Regarding settings.alphabetSearch, it has been implemented like that in the original code so that settings could be accessed in search and order plug-ins. I'm not sure what you mean how using local variable could be faster.

I really don't mean to introduce something that you don't want in the code base. If that is so, please don't hesitate to let me know. I will just host this code in a separate repository.

@DataTables
Copy link
Collaborator

Thanks for your reply. I appreciate your thoughts on this. I think, for the moment I would prefer that this plug-in remain just for filtering and additional options can be configured as people need.

I won't take this PR as it is, but if I get a chance I'll bring in the i18n and numbers aspect, as I think those are really useful additions.

@mpryvkin
Copy link
Contributor Author

mpryvkin commented Apr 28, 2016

I understand. I think I will create a separate repository to host this version of the plug-in retaining all the copyright and license of course.

@mpryvkin mpryvkin closed this Apr 28, 2016
@mpryvkin
Copy link
Contributor Author

For the sake of reference, added a new repository at https://github.com/gyrocode/jquery-datatables-alphabetSearch

@DataTables
Copy link
Collaborator

Awesome - thanks!

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.

None yet

2 participants