Add targetNode to canDrag rule

[ackvf]

I have a BoxListContainer and BoxListItem

The BoxListContainer is set to only accept BoxListItems

BoxListContainer.craft.rules.canMoveIn = incomingNode => incomingNode.data.type === BoxListItem

Now I need to also set that BoxListItem can be dragged only into BoxListContainer. How do I do that?

canDrag(currentNode: Node, helpers: NodeHelpers) => boolean
canMoveIn(incomingNode: Node, currentNode: Node, helpers: NodeHelpers) => boolean
canMoveout(outgoingNode: Node, currentNode: Node, helpers: NodeHelpers) => boolean

I am missing targetNode in canDrag, the other two have both nodes available.

this:

canDrag(currentNode: Node, targetNode: Node, helpers: NodeHelpers) => boolean

BoxListItem.craft.rules.canMoveIn = (_currentNode, targetNode) => targetNode.data.type === BoxListContainer

[prevwong]

The other 2 Canvas-specific rules exposes the incoming/outgoing child Node and their own corresponding Node. The canDrag rule only deals with the Node that's trying to move, thus only the corresponding Node is exposed to the method.

For your use-case, you can use the NodeHelpers instead:

canDrag: (node, helpers) => {
   return helpers(node.parent).get().data.type == BoxListContainer
}

[ackvf]

Nice, I noticed the helpers there and thought I could get the target node, thanks for pointing out node.parent to me. That will solve my issue.

Anyway, on the other hand, wouldn't it make sense to make the signature of all rules similar? After all, dragging is always done in relation, so I think it makes sense to put

1. the currently dragged element
2. the element (or perhaps only Canvas element, otherwise null) it hovers over

To defend this proposal, I think it will actually be quite common to do both:

1. set Canvas to accept certain Components
2. limit a Component to be droppable to certain Canvases

With this change, it will be much easier to write the logic, so instead of having to update, say, 3 Canvases to accept a new Component, we can simply implement the logic in the Component itself.

// now - 15 LOC

CardTop.craft = {
  rules: {
    canMoveIn: incomingNode => incomingNode.data.type == Text
  }
}

Carousel.craft = {
  rules: {
    canMoveIn: incomingNode => incomingNode.data.type == Text
  }
}

TextContainer.craft = {
  rules: {
    canMoveIn: incomingNode => incomingNode.data.type == Text
  }
}

// after - 5 LOC

Text.craft = {
  rules: {
    canDrag: (hovering, hovered) => [CardTop, Carousel, TextContainer].includes(hovered.data.type)
  }
}

With that being said, I would also suggest to change the terminology to

canMoveIn: (hovering, hovered) => boolean
canMoveOut: (hovering, hovered) => boolean
canDrag: (hovering, hovered) => boolean

So one does not need to think in terms of current, incoming, outgoing. In fact, when I was writing the other issue #27 I was confused a little bit when figuring out what is currentNode in the context of dragging a Component out of Canvas. I assumed current is the one "I am currently dragging" and that outgoingNode is the Canvas I am taking that element from. Then when I read the next sentence, I realised this is probably wrong and that the callback is only called on the Canvas, hence reversing the meaning of current and outgoing.

Anyway, if you were to adopt this terminology change, why not allow calling all three rules on both Canvases and Components? I think it makes perfect sense. See this use case:

I have a text component which I want to be draggable if it contains text "hello",
then I want to make it droppable only to the root Canvas and one additional Canvas, not any other,
and then I actually want to prevent the component from being dragged if it is inside the LockBox

canDrag: (hovering, hovered) => hovering.data.text === "hello"
canMoveIn: (hovering, hovered) => hovered.data.type === RootCanvas || hovered.data.type === LockBox
canMoveOut: (hovering, hovered) => !(hovered.data.type === LockBox)

Without the change, it would have to be written like this

// Text Component
canDrag: (hovering, helpers) =>
  hovering.data.text === "hello"
  && (
    helpers(hovering.parent).get().data.type === RootCanvas
    || helpers(hovering.parent).get().data.type === LockBox
  )
  && !( // i
    helpers(hovering.parent).get().data.type === LockBox
    && helpers(hovering.parent).get().data.children.includes(Text)
  )

i) this one is tricky, since we only want to prevent drag when the Text is already a child of LockBox, not when it is being just dragged into.

And the logic for the rules would be relatively simple

A Component that is Hovering

rules

• call canDrag if user grabs it and drags it around
• call canMoveIn when it hovers over a Canvas that accepts this kind of element (meaning, resolve Canvas rules first)
• call canMoveOut when it is a child of Canvas and user starts dragging it (resolve Canvas rules first)

### use cases and call order, only continue to next rule if the previous is true

• start dragging from Canvas (canDrag & canMoveOut)

  1. call canDrag // if true, continue
  2. call Canvas.canMoveOut
  3. call canMoveOut

• drag Component to the destination Canvas (canDrag & canMoveIn)

  1. call canDrag // I assume this continues to be fired
  2. call Canvas.canMoveIn
  3. call canMoveIn

A Canvas that is being Hovered

rules

• call canMoveIn when a Component hovers over it
• call canMoveOut when a Component that is a child of it starts dragging

### use cases and call order
It's the same as above, but to make something clear, these are not invoked by the Canvas, but instead, the Component resolves its Component.canDrag and only then it asks the underlying Canvas to resolve and return its Canvas.canMoveOut, then the Component resolves its Component.canMoveOut.

---

Oh man, I have gone a little wild here.

PS: I have just realised a Canvas can be draggable as well, so it will have to have separate canMoveIn rules for both 1) it is a Hovered Container, 2) it is a Hovering Component.
Either one would have to implement conditions

canMoveIn: (hovering, hovered) => {
  if (hovering === this) { }
  if (hovered === this) { }
}

or
Actually, I like the condition pattern. I don't want to make things complicated. If user makes a Canvas draggable, that's their choice, they probably know what they are doing, so adding a simple if statement isn't much of an overhead

Just for record, this would have been the other suggestion.

rules : {
  canDrag
  canMoveIn // this still fires when the Canvas is a container
  canMoveOut
  draggableCanvas: { // only used in draggable Canvas
    canMoveIn // this fires when the Canvas is dragged as component
    canMoveOut
  }

[prevwong]

You really went a little wild here 😂 But I get what you're saying.

After all, dragging is always done in relation

The canDrag rule is to check if a component can be dragged at present, thus this rule has nothing to do with any other Nodes but its own. It's not the same as your example which resembles more to what would have been a "canDrop" rule.

The reason why there is no canDrop rule or why as you suggested that those Canvas-specific rules are not available to non-Canvas components is because I prefer the idea of letting (Canvas) maintain control of what goes in and out of their Component rather than letting their children or incoming components decide. I believe that this is more straight-forward, and less issues will be come about from this.

With that being said, I would also suggest to change the terminology

In regards to the current implementation, I disagree with changing incoming and outgoing, because it is descriptive and it is literally what it means: an incoming Node and an outgoing Node. However, I agree with changing current. I think a better name should be used to reflect that it is the corresponding Node of the component that the rule is being applied to, perhaps something like self.

[ackvf]

Then you really suggest to use a hack to prevent a Component to be droppable anywhere but one Canvas? This use case is actually more common than having a Canvas decide which components it can accept.

Given I have a BoxItem Component and a BoxContainer Canvas, I need to prevent BoxItem to be droppable anywhere but into BoxContainer. I would literally have to edit every single Canvas in every possible Component to make sure it will not accept BoxItem.

And if I were to import external component, which I don't even have a control over, then there wouldn't really be a solution whatsoever (other than the canDrag hack).

Forgive me, but this is not right.

[ackvf]

Actually, the workaround doesn't help me

// BoxItem
return helpers(node.parent).get().data.type == BoxListContainer

I need the target element I am hovering over. If I am currently dragging a BoxItem, it's not a child of the BoxListContainer yet.

Any suggestions?

[prevwong]

It seems to me now that you're interested in implementing a canDrop rule which is not related to the canDrag rule that you created this issue for. Hence, this whole misunderstanding is happening now.

But anyways, with that brief example of yours, a canDrop rule might make sense. I would be glad to review a PR for this.

If I were to import external component, which I don't even have a control over

If you were to import an external component and you want to use as a <Canvas />, you will have to create a wrapper for it anyway in order for you to specify configuration for it.

p.s. Stop spamming the issues tracker. We can use Discord for more long-winded discussions such as this. It also doesn't help that you occasionally post your questions/issues on both platforms.

[ackvf]

My company blocks Discord since yesterday 😆

At first, my idea was to post some long texts and examples here and do the quick instant messaging on discords with reference to the issue here.

canDrop sounds reasonable. Actually, canDrag in tandem with canDrop can substitute my proposed canMoveOut and canMoveIn (and are in fact better, easier to understand), but that would still require to have the parent in both anyway.

Allright, let me take a look at it. I will try to implement that.

[prevwong]

but that would still require to have the parent in both anyway.

canDrop should have the target parent be passed to the method, yes.
But why would canDrag need the parent to be passed ? It's not essential and it's obtainable via the helper methods.

[ackvf]

Because there is a use case to lock a Component inside a Canvas, but keep the logic coupled to the component, not the Canvas.

It's similar to how I envision the Canvas.canMoveIn with Component.canDrop work together on deciding whether a Component will drop onto a Canvas.

I believe that both Canvas and Component should agree on the Component leaving. if both Canvas.canMoveOut and Component.something return true, the component will be allowed to leave.

But I understand that canDrag's original purpose was to only reflect Component's internal state and return true if it decides it's draggable. Perhaps another rule should contain the logic to allow it leave its parent Canvas? Pehraps canDrag, canDrop, canLeave ?

[prevwong]

Isn't this use case supported by canDrag already ? For example, let's say we want our component to only be able to move out depending on the parent it is in:

canDrag: (node, helpers) => {
  const parent = helpers.get(node.parent);
  if ( parent.data.type == BoxListContainer ) return false;
  return true;
}

Currently, Craft depends on both the component's canDrag and the target parent's canMoveOut for the component to be successfully moved out, so I think this already does what you want, or am I missing something ?