PoC: Table Drag & Drop #9955
PoC: Table Drag & Drop #9955
Conversation
| @@ -463,6 +483,88 @@ class Table extends UI5Element { | |||
| this.fireEvent<TableRowClickEventDetail>("row-click", { row }); | |||
| } | |||
|
|
|||
| _ondragenter(e: DragEvent) { | |||
There was a problem hiding this comment.
_ondragenter, _ondragover, _ondragleave are pretty much the same implementation as in List.ts.
So maybe there is a way to reuse the code in a Utility or something?
| * @public | ||
| */ | ||
| @property({ type: Boolean }) | ||
| movable = false; |
There was a problem hiding this comment.
technically not required, as you could just simply set draggable="true" directly on the ui5-table-row as it is in the light DOM.
But I guess, this would make sense for consistency across the API
There was a problem hiding this comment.
Yes, please use movable for consistency. It is also more meaningful in conjunction with the:
moveandmove-overevents- potential keyboard support from your side
Last but not least, it won't work like that if you put the attribute to the root element in the shadow DOM.
DonkeyCo
requested review from
a team and
simlin
and removed request for
a team
|
Please revisit the commit history, 112 files appear as changed. |
- add DragRowUtil, which provides reusable code for dragOver and drop in List and Table
- List and Table have a lot of redundant code regarding drag&drop - extract dragover and drop into utility functions
There was a problem hiding this comment.
Other test pages follow the pattern ComponentNameDragAndDrop.html. Please call this TableDragAndDrop.html
| function tableMove(e) { | ||
| const { source, destination } = e.detail; | ||
|
|
||
| if (source.element.hasAttribute("ui5-table-row") && destination.element.hasAttribute("ui5-table-row")) { |
There was a problem hiding this comment.
Better do such validations in tableMoveOver. It is misleading to show drop indicator and then to cancel the operation.
| if (sourceIndex === -1 || destinationIndex === -1) { | ||
| return; | ||
| } |
| * @public | ||
| */ | ||
| @property({ type: Boolean }) | ||
| movable = false; |
There was a problem hiding this comment.
Yes, please use movable for consistency. It is also more meaningful in conjunction with the:
moveandmove-overevents- potential keyboard support from your side
Last but not least, it won't work like that if you put the attribute to the root element in the shadow DOM.
| if (this.movable) { | ||
| this.setAttribute("draggable", "true"); | ||
| } else { | ||
| this.removeAttribute("draggable"); | ||
| } |
There was a problem hiding this comment.
Better do this in TableRow.hbs, to encapsulate this attribute in a shadow DOM.
| @@ -72,6 +78,17 @@ type TableRowClickEventDetail = { | |||
| row: TableRow, | |||
| }; | |||
|
|
|||
| type TableMoveEventDetail = { | |||
There was a problem hiding this comment.
You should also describe the move and move-overs events using @event decorator
| } | ||
|
|
||
| export { | ||
| DragOverResult, |
There was a problem hiding this comment.
Seems unnecessary to export DragOverResult
There was a problem hiding this comment.
I like the idea of this util. Please move both functions to separate files. Something like handleDragOver.ts and handleDrop.ts
| const dragOverResult: DragOverResult = { | ||
| targetReference: null, | ||
| placement: null, | ||
| }; | ||
|
|
||
| const draggedElement = DragRegistry.getDraggedElement(); | ||
|
|
||
| if (!(e.target instanceof HTMLElement) || !draggedElement) { | ||
| return dragOverResult; | ||
| } | ||
|
|
||
| const closestPosition = findClosestPosition( | ||
| items, | ||
| e.clientY, | ||
| Orientation.Vertical, | ||
| ); |
There was a problem hiding this comment.
Although this code is the same for List and Table, it is not for the Tree and TabContainer. Maybe put this back to the components and only reuse the code below?
| return dragOverResult; | ||
| } | ||
|
|
||
| function dropRow(e: DragEvent, component: UI5Element, target: HTMLElement, placement: `${MovePlacement}`): void { |
There was a problem hiding this comment.
Thumbs up for making this reusable. It will foster proper maintenance of the DragRegistry and can be directly reused by Tree and TabContainer.
This is a PoC to see what effort needs to be taken for D&D to be possible in the Table.