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

Callback support for acceptFrom #646

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

Conversation

mike16889
Copy link
Contributor

@mike16889 mike16889 commented Jan 22, 2021

Add ability to supply a function to decide if a dragged element can be dropped onto the sortable. This will fix #643

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
@mike16889
Copy link
Contributor Author

@lukasoppermann honestly, I'm in over my head with this one. I am not very familiar with npm.

Copy link
Owner

@lukasoppermann lukasoppermann left a 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.
Copy link
Owner

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`:
Copy link
Owner

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){
Copy link
Owner

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.')
Copy link
Owner

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) {
Copy link
Owner

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.

@lukasoppermann
Copy link
Owner

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.

@lukasoppermann lukasoppermann self-assigned this Aug 30, 2022
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.

ability for 'acceptFrom' to accept a function as an argument
2 participants