-
Notifications
You must be signed in to change notification settings - Fork 7
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
implemented multiple properties view #79
base: master
Are you sure you want to change the base?
Conversation
Few fixes for requirejs config to make JointJS load properly. Also jQuery updated to 2.2.4.
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.
Good. Minor fixes requested.
@@ -12,7 +12,9 @@ export class DefaultDiagramNode implements DiagramNode { | |||
private constPropertiesPack: PropertiesPack; | |||
private changeableProperties: Map<String, Property>; | |||
private imagePath: string; | |||
private propertyEditElement: PropertyEditElement; | |||
private propertyEditElements: PropertyEditElement[]; | |||
private delta = 20; |
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.
Delta of what? Not very helpful name.
private propertyEditElement: PropertyEditElement; | ||
private propertyEditElements: PropertyEditElement[]; | ||
private delta = 20; | ||
private topIndent = 30; |
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 name too.
setPropertyEditElementsPosition(x : number, y : number) : void { | ||
let propertiesCount = 0; | ||
|
||
for (let i in this.propertyEditElements) { |
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 can either use here for ... of
construction or use i
variable also as propertiesCount
this.propertyEditElement.setPosition(propertyEditElementX, propertyEditElementY); | ||
var propertyEditElementY = parentPosition.y + this.boundingBox.height - this.topIndent; | ||
|
||
for (var propertyKey in this.changeableProperties) { |
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.
As IDEA reminds: it's better to use let
instead of var
cause it behaves more strictly.
initPropertyEditElements(zoom: number): void { | ||
var parentPosition = this.getJointObjectPagePosition(zoom); | ||
this.propertyEditElement = new PropertyEditElement(this.logicalId, this.jointObject.id, | ||
this.changeableProperties); | ||
var propertyEditElementX = parentPosition.x + (<number> (this.boundingBox.width - 50)/2); |
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.
Extract magic number to appropriate constant.
node.getPropertyEditElement().getHtmlElement().remove(); | ||
if (node.getPropertyEditElements()) { | ||
var editElements = node.getPropertyEditElements(); | ||
for (let i in editElements) { |
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.
for ... of
here maybe?
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 everywhere below
@@ -121,8 +121,11 @@ export class DiagramScene extends joint.dia.Paper { | |||
}); | |||
|
|||
node.getJointObject().remove(); | |||
if (node.getPropertyEditElement()) { | |||
node.getPropertyEditElement().getHtmlElement().remove(); | |||
if (node.getPropertyEditElements()) { |
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.
What does it mean? Is it check for null or what? Use explicit syntax.
@@ -159,8 +162,11 @@ export class DiagramScene extends joint.dia.Paper { | |||
this.nodesMap[node.getJointObject().id] = node; | |||
this.graph.addCell(node.getJointObject()); | |||
node.initPropertyEditElements(this.zoom); | |||
if (node.getPropertyEditElement()) { | |||
node.getPropertyEditElement().getHtmlElement().insertBefore("#" + this.getId()); | |||
if (node.getPropertyEditElements()) { |
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.
Here also be more explicit.
…ties # Conflicts: # editor-core/src/main/webapp/app/core/editorCore/model/DefaultDiagramNode.ts # editor-core/src/main/webapp/app/core/editorCore/model/DiagramNode.ts
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.
Please repair:
- Changing of proprerty will move it to default postion
- Saving of property will move it to default position
Fixes requested.
changeTextPosition() : void { | ||
var dx = this.getX() - this.lastPointermoveCursor.x; | ||
var dy = this.getY() - this.lastPointermoveCursor.y; | ||
console.log("Diagram pos in changeTextPosition : " + this.getX() + ", " + this.getY()); |
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 do not use log levels, so it's better to remove unnecessary logs before merge to master.
private jointObject: ImageWithPorts; | ||
|
||
/** | ||
* Name and type of diagramm. |
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.
Write one doc per one field.
private constPropertiesPack: PropertiesPack; | ||
|
||
/** | ||
* Properties, which can be changes by user. |
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.
changeD
private changeableProperties: Map<String, Property>; | ||
|
||
/** | ||
* Path to image. |
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.
Not really helpful. What kind of path? Filepath, url?
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 no idea.
private propertyEditElements: Map<String, PropertyEditElement>; | ||
private parentNode: DiagramContainer; | ||
|
||
/** | ||
* Graph, where diagram is located. Used to set new properties on the screen. |
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.
Didn't understand it.
pointermove(cellView, evt, x, y): void { | ||
console.log("Default diagram node pointer move with x : " + x + " and y : " + y); | ||
cellView.options.interactive = true; | ||
var diffX = x - this.lastMousePosition.x; | ||
var diffY = y - this.lastMousePosition.y; | ||
this.lastPointermoveCursor.x = this.getX(); | ||
this.lastPointermoveCursor.y = this.getY(); | ||
this.lastDiagramPosition.x = this.getX(); |
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.
Position of the whole diagram? Maybe lastCursorPosition?
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.
Position of the whole diagram? -- Yes. Coordinates of underlying joint object.
No description provided.