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

jQuery compatibility? #151

Closed
dandv opened this issue Dec 5, 2014 · 11 comments
Closed

jQuery compatibility? #151

dandv opened this issue Dec 5, 2014 · 11 comments
Labels
enhancement Quality of life changes to existing features

Comments

@dandv
Copy link
Contributor

dandv commented Dec 5, 2014

I found myself writing, out of habit, new Sortable($('#foo')).

Would it be bad if Sortable supported jQuery? The change would be small, I think. Something like this wrapping the constructor:

if (el.jquery) [].concat($.get(el)).forEach( function (el) {
...
}
@FezVrasta
Copy link

why don't you write $("#foo")[0] ?

@RubaXa
Copy link
Collaborator

RubaXa commented Dec 5, 2014

Because it returns an instance, so it is not clear what to do in collection hang at first element or all?

@dandv
Copy link
Contributor Author

dandv commented Dec 5, 2014

The jQuery convention is to preserve method chaining, so the entire collection should be processed, and the resulting Sortable objects should be returned in an array.

@RubaXa
Copy link
Collaborator

RubaXa commented Dec 5, 2014

This is not correct behavior for new.

Correctly will make, ex. so:

if (typeof jQuery !== 'undefined') {
    jQuery.fn.sortable = function (options) {
      return this.each(function () {
          var $el = $(this);
          var sortable = $el.data('sortable');

          if (!sortable && options instanceof Object) {
              sortable = new Sortable(this, options);
              $el.data('sortable', sortable);
          }

          if (sortable && (options in sortable)) {
               sortable[sortable].apply(sortable, [].slice.call(arguments, 1));
          }
      }) 
    };
}

@dandv
Copy link
Contributor Author

dandv commented Dec 5, 2014

That looks much better.

@njh
Copy link

njh commented Dec 30, 2014

👍 Would be great to have some jQuery support to make things a bit tidier if already using jQuery in a project

RubaXa added a commit that referenced this issue Dec 31, 2014
@RubaXa
Copy link
Collaborator

RubaXa commented Dec 31, 2014

@njh
Copy link

njh commented Dec 31, 2014

Tested and working great for my limited usage.

I see you updated Sortable.min.js - was this supposed to include the jquery bindings?

@RubaXa
Copy link
Collaborator

RubaXa commented Dec 31, 2014

I see you updated Sortable.min.js - was this supposed to include the jquery bindings?

No.

@RubaXa RubaXa added the enhancement Quality of life changes to existing features label Dec 31, 2014
RubaXa added a commit that referenced this issue Dec 31, 2014
@RubaXa
Copy link
Collaborator

RubaXa commented Dec 31, 2014

@njh + grunt jquery:min

@njh
Copy link

njh commented Dec 31, 2014

Fantastic!

@RubaXa RubaXa closed this as completed Jan 5, 2015
RubaXa added a commit that referenced this issue Jan 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Quality of life changes to existing features
Projects
None yet
Development

No branches or pull requests

4 participants