-
Notifications
You must be signed in to change notification settings - Fork 36
feat: ability to drag and drop tiles #1297
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
Conversation
… and new index of the dragged item
…h to lib/diagram/drag-tiles refactor(lib-client): Implemented a service to incorporate the logic to find the parent id into it. reverted changes to row-index.service.ts and tile-guards.ts as we can get the branch tile id of a row on the run.
…ion of the tile and removed unused assignment in the template
…ions to a @flogo-web/assets library. Fixed format issues.
…e each row will have preDrop, drop and postDrop zones
… children. Also disabling the drop after such element
…ty space from flow specific to drag-drop-service. Enabled the feature to streams
…hed from the same tile - restrict immediate parent tile to drop into its child row - restrict immediate parent tile row's parent tile to drop into its children rows
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.
Other than the following comments I do not find issue with the implementation. We can consider the review comments as a separate change as they will take a bit of time to implement.
libs/lib-client/diagram/src/lib/diagram/diagram-row.component.ts
Outdated
Show resolved
Hide resolved
libs/lib-client/diagram/src/lib/diagram/diagram-row.component.ts
Outdated
Show resolved
Hide resolved
@@ -77,4 +77,43 @@ export class DragTileService { | |||
parentId, | |||
}; | |||
} | |||
|
|||
getAllRowsParentTiles(tileMatrix, rowIndexes) { |
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.
We ask consuming component to call this method with Graph Nodes and we can initialise an empty map or a map<string, rowIndexes: number[]> if getRowIndexOf is provided as a parameter
getAllRowsParentTiles(tileMatrix, rowIndexes) { | |
initializeRelationsLookupTable(nodes: NodeDictionary, getRowIndexOf?: (string) => number) { |
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.
Agree, but a resource will not consume this method if it cannot have multiple rows. So i think this condition is not necessary 🤔
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.
IMO, Every resource must call this initialization service method. Else, we can miss an aspect of resetting the data. Think about it and plan accordingly
}; | ||
|
||
/* | ||
Case 1. If root element is moved to different place. It will not have a previous parent and it will have previous child. |
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 really like that the reasons behind the logic are explained here 👍
const prevParentId = nodeToMove.parents.pop(); | ||
const prevChildId = nodeToMove.children.find(findTaskId); | ||
const newParentNode = newParentId && allNodes[newParentId]; | ||
const newChildId = newParentNode |
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.
Don't change this. I'm just wondering if the new syntax makes this easier or confusing?
newParentNode?.children.find(findTaskId) ?? graph.rootId
const findTaskId = taskIdFinder(graph.nodes); | ||
const allNodes = graph.nodes; | ||
const nodeToMove = allNodes[movingItemId]; | ||
const prevParentId = nodeToMove.parents.pop(); |
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.
you're mutating an object and because we're using the reducer pattern that should be avoided. Updates should be immutable.
const prevParentId = nodeToMove.parents[0];
nodeToMove = {
...nodeToMove,
parents: []
}
nodeToMove.children = replaceNodeIds(nodeToMove.children, prevChildId, newChildId); | ||
if (newParentId) { | ||
nodeToMove.parents.push(newParentId); | ||
newParentNode.children = replaceNodeIds( |
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 is also mutation
|
||
return { | ||
...state, | ||
[currentGraphName]: updateLinks(currentGraph, itemId, parentId), |
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've mentioned it in the updateLinks file but there's an issue with the fact that updateLinks
function is not a pure function (it mutates parameters) and it is being used in reducers
awesome! 👏🎉 |
Description
Ability to reorganize the diagram by drag and dropping tiles
partially fixes #707
Motivation and Context
This feature, enabling drag and drop tiles will allow user to rearrange activities in the diagram with ease
How has this been tested?
Changes are manually tested
Types of changes
Checklist: