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

Extendable tablesort #75

Merged
merged 2 commits into from Feb 23, 2015
Merged

Extendable tablesort #75

merged 2 commits into from Feb 23, 2015

Conversation

xPaw
Copy link
Contributor

@xPaw xPaw commented Feb 22, 2015

This is for #74.

There's one problem with this approach though, because dotsep/filesize sorts get loaded later, they are not considered for sorting because number sort catches them first. There should be some kind of priority or something, I'm not entirely sure.

I've also added names for each sorting method, so it could also be used to fix #53.

@xPaw
Copy link
Contributor Author

xPaw commented Feb 22, 2015

Hmm, tests passed for that, however I am sure filesize/dotsep sorting is wrong. These tests should be extended to cover more cases.

@tristen
Copy link
Owner

tristen commented Feb 22, 2015

Awesome @xPaw! Before we get this in:

  • Should we also move dates and currency out as well?
  • Priority is totally going to be an issue ... I too can't think of a cleaner way than setting a priority index and it would be a very good idea not to set this index in the extend functions themselves. I wonder if:
    • We move every pattern/sort into separate files (core would have none) and priority is set by the order it's included in the HTML?
    • Priority is set as an option by users implementation? like sortorder: ['dotsep', 'number'] not huge on this idea.

This is a breaking change & should be tagged v3.0.0. I can jump on documentation and writing better coverage for our tests.

@xPaw
Copy link
Contributor Author

xPaw commented Feb 22, 2015

Yeah I guess the other two sorts can be moved out as well, and that would kinda fix the priority issue (you would prioritize by including the files in order you need). I also thought about allowing to specify which sort method to use per column (via data-sort-method or something similar).

@tristen
Copy link
Owner

tristen commented Feb 22, 2015

I also thought about allowing to specify which sort method to use per column (via data-sort-method or something similar).

This is a great idea & addresses #53 nicely. That would probably be a good change in addition to splitting all sorts out of core.

@xPaw
Copy link
Contributor Author

xPaw commented Feb 22, 2015

I've added that, needs testing.

@xPaw
Copy link
Contributor Author

xPaw commented Feb 22, 2015

@tristen Do you know if these lines are still needed, because I think they are not.

in getInnerText:

            if (typeof el === 'string' || typeof el === 'undefined') {
                return el;
            }

And as far as I know, textContent doesn't return HTML comments.

                // Exclude cell values where commented out HTML exists
                if (item.substr(0, 4) !== '<!--' && item.length !== 0) {
                    items.push(item);
                }

@tristen
Copy link
Owner

tristen commented Feb 22, 2015

Yep! looks like those typeof checks can go now.

textContent should return HTML comments no? http://mistakes.io/#1be232a56d8a684ab428

@xPaw
Copy link
Contributor Author

xPaw commented Feb 22, 2015

That's because you assigned it as a string: http://jsfiddle.net/thxefs90/

@tristen
Copy link
Owner

tristen commented Feb 22, 2015

Oh derp: http://mistakes.io/#fc11895cd90f7ae05bfa Welp looks like that can go too!

@tristen
Copy link
Owner

tristen commented Feb 22, 2015

Awesome work! I'll add words and tests a little later today.

Move out date/number sorting options

Move things around

Add data-sort-method

Rename events for #66

Remove unnecessary checks

Remove unnecessary getInnerText call

Extendable tablesort
@xPaw
Copy link
Contributor Author

xPaw commented Feb 22, 2015

I squashed my changes into one commit.

- Split tests out into separate spec scripts
- Write better test coverage. Closes #44
- Add demo for `beforeSort` &  `afterSort` events. Ref #66
- Move to 2 space indentation. (Sorry, needed to happen)
  - Add .editorconfig to stabilize this convention for editors that support it.
- Document extending Tablesort with custom function
- Document data-sort-method. Ref #53
- Fix refresh method when triggering a refresh
tristen added a commit that referenced this pull request Feb 23, 2015
@tristen tristen merged commit 8ea6b4d into gh-pages Feb 23, 2015
@tristen tristen deleted the extendable branch February 23, 2015 04:48
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.

override sort algorithm for specific column
2 participants