-
Notifications
You must be signed in to change notification settings - Fork 62
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(drag-n-drop): add item dragging into grid #111
base: main
Are you sure you want to change the base?
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.
Thanks for the PR submission @skutam !!
I have made a first review with some of the main points to discuss/act upon. Here are some additional suggestions:
-
I would suggest adding the 'drag from outside' functionality in a new example page, rather than in the playground ones. It could be named 'drag-from-outside.' This way, the functionality is isolated in an example, making it easier for users to understand (I can do this part if you want)
-
I would also suggest removing the
ktd-
prefix from the file names
We can extend it on the Discord if needed. I truly appreciate your effort on this task. I would like to make the PR merging process as fast as possible, but in this case, due to the significant changes, I expect it will take some time... But we will definetly merge it!
|
||
return this.startDragSequence$(source, event, 'drag').pipe( |
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 have doubts about initiating the drag sequence within the grid at the moment the ktdDrag
item begins dragging. Initially, it appears more logical to start the drag sequence within the grid once the ktdDrag
item enters the grid.
I think it will also simplify the 'Scroll Offset Support' for the ktdDrag
directive, as we can then subscribe to scroll events exclusively on the ktd-grid
where the drag action is occurring.
).subscribe(({layout, item, type}) => this.stopDragSequence({layout, item, type})), | ||
this.gridDragItemRegistry.ktgDragItems$.pipe( | ||
startWith(this.gridDragItemRegistry.ktgDragItems$.value), | ||
switchMap((draggableItems) => { |
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.
It doesn't seem very scalable for every ktdDrag to be connected to all available ktdGrids. It would be nice to add a connectedTo
Input property to the ktdDrag
, and in this code filter only the ktdDrag
directives that are connected to this grid.
We could consider adding this feature in a subsequent iteration though, no worries for now 👌
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.
Ok I created new connectedTo
attribute for ktd-drag
, has the same use as drag-n-drop
in angular cdk
.
if (this.gridService.draggingItem !== null && !this.isPointerInsideGridElement(pointerDragEvent)) { | ||
this.destroyPlaceholder(); | ||
return; | ||
} |
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.
This code will disappear if we initiate the drag sequence as soon as the ktdDrag enters the grid, and terminate the drag sequence when the ktdDrag leaves the grid
newLayout = [...newLayout, { | ||
...this.gridService.draggingItem.layoutItem, | ||
id: this.getNextId(), | ||
}]; |
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 would prefer to let the user handle the drop action manually when it comes from outside. Similar to this example: https://material.angular.io/cdk/drag-drop/overview#cdk-drag-drop-connected-sorting
I believe this approach is more robust and flexible. The only drawback is that it requires the user to write more code... but overall, I see more benefits than cons. Here are some:
- Forces to push a [layout] with the added item, ensuring consistent state synchronization.
- Allows to omit the drag for specific reasons, and not add the dropped item to the [layout]
- Enables the user to display a dialog or an intermediate step before adding the item to the grid.
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 have created dropped
event on the ktd-grid
that fires when item with ktd-drag
directive is dropped on it. The event returns
interface KtdDroppedEvent {
event: MouseEvent | TouchEvent;
layout: KtdGridLayout;
layoutItem: KtdGridLayoutItem;
data: any;
}
* Still dont know how we would handle nested scrollParents, generated by nested grids. | ||
*/ | ||
|
||
this.element.style.transform = `translate3d(${currentX}px, ${currentY}px, 0)`; |
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.
Currently, ktdDrag manages its drag movement, whereas the drag movement of the ktd-grid-item is controlled by the ktd-grid. This creates a bit of inconsistency. I believe that the approach you're taking here is correct, so we should enable the ktd-grid-item to handle its drag movement as well.
This will also open the door for drag-and-drop functionality between grids in the near future.
this._ktgDragItems.splice(this._ktgDragItems.indexOf(item), 1); | ||
this.ktgDragItems$.next(this._ktgDragItems); | ||
} | ||
} |
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.
nice 👌
layoutItem: { | ||
id: source.id, | ||
w: source.width > 0 ? source.width : this.cols, | ||
h: source.height > 0 ? source.height : (this.height !== null ? this.height : 1), |
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 would suggest setting the default values to 1, instead of the full width or height.
…en we have multiple grids
Hi, Great library. Really happy with the functionalities. I'm looking for this feature. Will this be merged any time soon? Best, |
@mcrodriguezb hello it is currently partially done, basically it works but there are still bugs that I am fixing because it is a large feature that also includes drag n drop between grids. You can help with the testing if you want to, because I am currently trying to use it in my project and by using it am also testing it. |
Hello, Best regards. |
New Feature:
ktd-drag
Directive andDragRef
Class (Draft)Description
This pull request introduces the
ktd-drag
directive and theDragRef
class to enhance the Angular-grid-layout library. Thektd-drag
directive allows users to drag new items into the grid seamlessly. It integrates with the existing functionality of the Angular-grid-layout library and provides optional parameters, events, and support for custom drag data.The
DragRef
class serves as a foundational component, handling the logic for dragging operations. It includes features such as drag handles, drag start threshold, and manual drag initiation.Changes Made
ktd-drag
directive to facilitate dragging new items into the grid.DragRef
class to handle drag operations.ktd-drag
directive, such asdisabled
,dragStartThreshold
, anddraggable
.dragStart
,dragMove
, anddragEnd
.Main Problems (Marked with TODO:)
Problem: Scroll Offset Support in
ktd-drag
DirectiveThe current implementation lacks support for handling scroll offset during the drag operation. The
TODO:
comment suggests adding this functionality. To address this, consider creating observables for each scroll parent element that emit scroll offset values. Use these offsets when the dragged item is on top of the respective scroll parent. Be mindful of potential challenges with nested scroll parents, especially in the context of nested grids.Problem: Handling Large Custom Data
Another challenge is how to handle custom data in the
ktd-drag
directive, particularly when the data could contain large JSON. Passing large data directly to calculating functions may not be optimal. Explore alternative approaches, such as passing a reference or key that allows fetching the data when needed. Ensure comprehensive testing for scenarios related to large custom data.Usage
Developers can use the
ktd-drag
directive to enable dragging of new items from an external source and dropping them onto thektd-grid
. The directive supports various configurations and events to provide a flexible dragging experience.Testing
Test cases have been added to ensure the correct functionality of the
ktd-drag
directive and theDragRef
class. The implementation has been tested with different scenarios, including drag start, drag move, and drag end.Note to Reviewers
I appreciate your time and consideration in reviewing this pull request. The addition of the
ktd-drag
directive andDragRef
class aims to enhance the versatility of the Angular-grid-layout library. I am open to feedback and collaboration on any further improvements.