Skip to content

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

Merged
merged 21 commits into from
May 1, 2020
Merged

feat: ability to drag and drop tiles #1297

merged 21 commits into from
May 1, 2020

Conversation

ppaidi
Copy link
Contributor

@ppaidi ppaidi commented Apr 23, 2020

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes, no api changes)
  • Build/CI related changes
  • Documentation related changes
  • Other... Please describe:

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

apandura-tibco and others added 18 commits March 2, 2020 17:37
…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
Copy link
Collaborator

@apandura-tibco apandura-tibco left a 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.

@@ -77,4 +77,43 @@ export class DragTileService {
parentId,
};
}

getAllRowsParentTiles(tileMatrix, rowIndexes) {
Copy link
Collaborator

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

Suggested change
getAllRowsParentTiles(tileMatrix, rowIndexes) {
initializeRelationsLookupTable(nodes: NodeDictionary, getRowIndexOf?: (string) => number) {

Copy link
Contributor Author

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 🤔

Copy link
Collaborator

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.
Copy link
Collaborator

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
Copy link
Collaborator

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();
Copy link
Collaborator

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(
Copy link
Collaborator

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),
Copy link
Collaborator

@fcastill fcastill Apr 29, 2020

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

@fcastill fcastill merged commit 80f94df into master May 1, 2020
@fcastill fcastill deleted the drag-drop branch May 1, 2020 23:44
@fcastill
Copy link
Collaborator

fcastill commented May 1, 2020

awesome! 👏🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to reorganize a flow
3 participants