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

Fixed failing sorts after multiple calls to makesortable and extended matches for percent values #11

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

virgile-hogman
Copy link

  1. Fixed major issue where the sort would work only once for each column, when updating table content dynamically. Potentially many other side effects.
    Reason: multiple instances of same handler were stacked if makesortable is called several times without resetting the TH elements. This also happens if makesortable is called manually before init(). Typically the same handler could be registered twice and therefore cancelling the sort!
    Solution: no need to unregister the current handler. This can be managed very nicely giving a reference to a unique function (not anonymous) when calling addEventListener. See section "Multiple identical event listeners" here:
    https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener
  2. Fixed minor issue where "num %" values would not be sorted correctly if space or tabs chars were found before % char. Minor fix extending the regex.

1. Fixed issue where sort would work only once for each column, after updating table content dynamically. Reason: multiple handlers were stacked if makesortable was called several times on same table (or when makesortable called before init).
2. Fixed issue where "num %" values would not be sorted correctly due to space chars before %.
3. Reverse sort activated by default (using mostly percentages).
retro-fit current github version and remove reverse patch
@stuartlangridge
Copy link
Owner

There may be a documentation bug here; one doesn't need to call makeSortable multiple times, just once when table data is loaded. This whole area needs rework so that table sorting is more dynamic, which I plan to do at some point. But I'll happily leave your work here as a workaround for people suffering this problem before that rework is done; thank you!

@virgile-hogman
Copy link
Author

virgile-hogman commented May 3, 2016

Thanks for your fast answer! Sure makeSortable is not supposed to be called several times but in my case it makes sense. Let me show this example:

  1. page is loaded initially with a dynamic content generated in my own page ready(), lets say a function populateTable. This function calls makeSortable because it is reused later.
  2. just after that, init is called by sortable.js when page is ready, which calls makeSortableagain. The issue here is i don't have any control over this call, which comes implicitly after my own code(using standard jquery to handle the ready event).
  3. later, on other events, populateTable is called again with different parameters, generating a new content, hence the need to call makeSortable again.

Another workaround would be to add specific code in my populateTable to handle the case if init has been called or not but this is not a good solution. In any case calling makeSortableseveral times should not create these side effects, sortable.js should be more robust as proposed in my pull request :)
Hope this scenario helps.

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