-
-
Notifications
You must be signed in to change notification settings - Fork 461
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
Callback support for acceptFrom #646
base: master
Are you sure you want to change the base?
Conversation
Add ability for acceptFrom to accept a function returning true/false on whether the currently dragged dom node can be accepted by the sortable
Supporting changes for adding callback support to acceptFrom
Callback support for acceptFrom
@lukasoppermann honestly, I'm in over my head with this one. I am not very familiar with npm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mike16889,
thank you very much for sending the PR. It seems that your changes make some of the tests fail, so we will need to fix this before we can merge anything.
I am going to seem nit-picky, but I added a couple comments to the readme.md part to keep it more in sync with the projects coding style.
I will look more into the js changes later on.
You are writing you are especially struggling with npm
? Can you elaborate on your issues so I can try to help overcome any barrier.
@@ -244,6 +244,19 @@ In the example the current list `.sortable` allows items within it to be sorted | |||
|
|||
If you want to be able to move items between to sortables, the `acceptFrom` option must be present on both of them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is not you, but if you change it in any case, can you fix the typo
move items between to sortables
to
move items between two sortables
@@ -244,6 +244,19 @@ In the example the current list `.sortable` allows items within it to be sorted | |||
|
|||
If you want to be able to move items between to sortables, the `acceptFrom` option must be present on both of them. | |||
|
|||
Can also accept a callback function with `destinationSortable` and `draggedElement` as paramaters, This function should return `true` or `false`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please change this to:
You can also provide a function for the acceptFrom
option. The function will receive two parameters, destinationSortable
and draggedElement
. Your function must return a boolean value (true
or false
).
|
||
``` javascript | ||
sortable('.sortable', { | ||
acceptFrom: function(destinationSortable, draggedElement){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please change this to use a fat arrow function and change it to the below:
(destinationSortable, draggedElement) => {
// your custom function
if (draggedElement.classList.contains('canDrop')) {
return true
} else {
return false
}
}
// check if valid sortable | ||
if (destination.isSortable === true) { | ||
const acceptFrom = store(destination).getConfig('acceptFrom') | ||
// check if acceptFrom is valid | ||
if (acceptFrom !== null && acceptFrom !== false && typeof acceptFrom !== 'string') { | ||
if (acceptFrom !== null && acceptFrom !== false && typeof acceptFrom !== 'string' && typeof acceptFrom !== 'function') { | ||
throw new Error('HTML5Sortable: Wrong argument, "acceptFrom" must be "null", "false", or a valid selector string.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mike16889 please change this to:
'HTML5Sortable:` Wrong argument, "acceptFrom" must be "null", "false", a function returning a boolean, or a valid selector string.'
return true | ||
} | ||
// check if lists are connected with connectWith | ||
if (store(destination).getConfig('connectWith') !== undefined && store(destination).getConfig('connectWith') !== null) { | ||
return store(destination).getConfig('connectWith') === store(origin).getConfig('connectWith') | ||
if ((store(destination).getConfig('connectWith') !== undefined && store(destination).getConfig('connectWith') !== null) && store(dragging.parentElement).getConfig('connectWith') !== undefined && store(dragging.parentElement).getConfig('connectWith') !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mike16889 why dod you add this check for the parent? I think this may create an issue if you have a place that does not accept, but only provide items to drag.
Hey @mike16889 I went through the PR and pointed out some parts that I would love you to adjust. Once they are done we can look into the failing tests. |
Add ability to supply a function to decide if a dragged element can be dropped onto the sortable. This will fix #643