api.jqueryui.com icon indicating copy to clipboard operation
api.jqueryui.com copied to clipboard

Sortable - helper arg & return types unclear if jQuery or JS?

Open kkmuffme opened this issue 3 years ago • 5 comments

https://api.jqueryui.com/sortable/#option-helper

Function: A function that will return a DOMElement to use while dragging. The function receives the event and the element being sorted.

Does this function actually want a native JS Element or a JQuery<HTMLElement> returned?

and the element being sorted.

This means the 2nd argument is JQuery<HTMLElement> or a native JS Element ?

Thanks for clarifying this, ideally it would also be updated in the docs page (I can PR an update in the TS @types once it's clear)

kkmuffme avatar Jan 03 '23 16:01 kkmuffme

Thanks for the report. I transferred the issue to the API repo.

I agree the wording here is a bit confusing. The function accepts a native DOM element as documented. As for the return value, both a native element and a jQuery one will work as the return value is being wrapped with jQuery: https://github.com/jquery/jquery-ui/blob/1.13.2/ui/widgets/sortable.js#L1131

Unfortunately, this whole code is untested: https://github.com/jquery/jquery-ui/blob/1.13.2/tests/unit/sortable/options.js#L359-L361. 🤦🏻 PRs with tests are welcome! So do PRs against the docs repo (i.e. this one).

mgol avatar Jan 11 '23 15:01 mgol

The function accepts a native DOM element as documented

This seems to be wrong, as it accepts a JQuery element only. See:

helper: function( unusedEvent, ui ) {
	if ( ui instanceof jQuery ) {
		console.log( 'jquery' );
	} else if ( ui instanceof HTMLElement ) {
		console.log( 'dom' );
	} else {
		console.log( 'else' );
	}

	return ui;
},

kkmuffme avatar Jan 12 '23 13:01 kkmuffme

I linked to the place where the options is read in my comment and you can see that a native element is passed to the function. If you think there's a case where this does not happen, please prepare a test case showing this; a raw comment snippet doesn't help me much.

mgol avatar Jan 12 '23 14:01 mgol

https://jsfiddle.net/ob0hgu32/

kkmuffme avatar Jan 12 '23 22:01 kkmuffme

Thank you! You are right, I read the code too fast. 🤦🏻 The native element being passed is the DOM element on which the widget was created - in your example case, the <ul> one; this can be seen here: https://jsfiddle.net/m_gol/mjuokzqe/1/

I'd appreciate a PR updating the docs!

mgol avatar Jan 12 '23 23:01 mgol