-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
feat: add validateDrop to mwlDroppable directive #111
feat: add validateDrop to mwlDroppable directive #111
Conversation
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 thanks for this, it's really solid work. My only remark would be to make the api a bit more flexible, other than that LGTM! Thank you! 🙌
* This is useful if you would like to prevent dropping when there are floating | ||
* elements (e.g. absolutely positioned) above the `mwlDroppable` element | ||
*/ | ||
@Input() restrictByEventTarget = 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 we rework the api a little bit to make it a bit more flexible. I'm thinking something like we do on the draggable directive where we let the user pass in a validateDrag
function that lets them control the behavior.
So the API would be something like:
export interface ValidateDropParams {
target: HtmlElement;
}
export type ValidateDrop = (params: ValidateDropParams) => boolean;
@Input() validateDrop: ValidateDrop;
and then this function can move into user land and you can have complete control over the behaviour in your own app:
private isTargetAllowed(target: EventTarget): boolean {
if (!this.restrictByEventTarget) {
return true;
}
const closestDroppableElement = (target as Element).closest(
'[mwlDroppable]'
);
return closestDroppableElement === this.element.nativeElement;
}
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.
Great idea! I'm happy to update it.
Do you think it's worth including a generic version of isTargetAllowed
as an out-the-box drop validator?
e.g.
type DropValidator = (params: ValidateDropParams) => boolean;
interface DropValidators {
restrictByEventTarget(selector: string) => DropValidator
}
import { DropValidators } from 'angular-draggable-droppable';
<div [mwlDroppable] [validateDrop]="DropValidators.restrictByEventTarget('[mwlDroppable]')">
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 sorry about the late reply, I've been out on vacation. I think as the logic is pretty simple it's better to keep it in userland for now, it could always be added as a feature to the library in the future if there was a big demand for it
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.
Thanks @mattlewis92, that makes sense. I actaully started making the change the other day and almost immediately thought the same thing, not worth adding it here atm.
I have updated the change to expose the suggested validateDrop
input. Please review when you have some time 😄
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.
Awesome, thank you! One last thing, could you add the 2 new interfaces here: https://github.com/mattlewis92/angular-draggable-droppable/blob/master/projects/angular-draggable-droppable/src/public_api.ts so that they can be used from the exported package as well
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.
done
@@ -33,6 +33,23 @@ export interface DropEvent<T = any> { | |||
dropData: T; | |||
} | |||
|
|||
export interface ValidateDropParams { | |||
/** | |||
* CientX value of the mouse location where the drop occurred |
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.
Minor nit, but there is a typo here
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.
🤦
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.
fixed
*/ | ||
clientX: number; | ||
/** | ||
* CientY value of the mouse location where the drop occurred |
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.
and here as well
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.
fixed
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.
Perfect, thank you!!
Add a new input to
DroppableDirective
that allows you to restrict the drop with a newvalidateDrop
function input. WhenvalidateDrop
is defined it will allow/disallow the drop event being fired. The event target fromthe mouse move event has also been piped through fromDraggableDirective
to be included inValidateDropParams