Allow using a Function as the cancel option instead of a Selector. #2324
Allow using a Function as the cancel option instead of a Selector. #2324kghandi wants to merge 1 commit intojquery:mainfrom
Conversation
|
|
mgol
left a comment
There was a problem hiding this comment.
Thanks for the contribution. This change makes sense to me, although before formally accepting I'd like @fnagel to also voice his opinion for possible wider context.
If Felix agrees this makes sense, there are still a number of changes we need before accepting this PR:
- Make sure you follow our style guide: https://contribute.jquery.org/style-guide/js/. Currently, indentation is wrong, it uses spaces instead of tabs, spaces are missing inside of parens, etc.
- Connected to the above - make sure our tests & linters succeed on the code before asking for another review. In this case, ESLint fails, for example. Run
npm install && npm run build && npm run lint && npm testlocally and see if everything works. - This change needs tests in all affected modules: draggable, sortable, etc. - perhaps whatever depends on
$.ui.mouse. - This needs API updates so we'll need a PR to https://github.com/jquery/api.jqueryui.com/ as well before merging any new API.
|
Sounds reasonable to me and the changes are not that big, so I'm fine with integrating this when all requirements are met. |
…is is particularly useful when using Sortable on nested lists.
05feb2a to
af50299
Compare
|
I have fixed the code style as requested. I have also created pull request for api documentation jquery/api.jqueryui.com#369 |
mgol
left a comment
There was a problem hiding this comment.
Thanks for the updates and the API PR. We still need to get this point addressed:
This change needs tests in all affected modules: draggable, sortable, etc. - perhaps whatever depends on
$.ui.mouse.
It's very important to have all new features covered by extensive tests.
This is particularly useful when using Sortable on nested lists.
I had a similar issue to that listed in issue #1996. Allowing cancel option to be a function allows me to override the default cancel implementation using a selector with something like:
$('.container', context).sortable({
items: '>.widget',
cancel: function (event) {
return $(event.target).closest('.widget').is('.locked');
}
});