Skip to content
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

New Added support for node label alignment #78

Open
wants to merge 1 commit into
base: release/1.0.0
Choose a base branch
from

Conversation

parmar-abhinav
Copy link

This PR introduces a new feature allowing users to have greater control over the node label alignment in the graph by defining custom not label positions i.e. top, left, right, bottom

@parmar-abhinav
Copy link
Author

@tonilastre Can you pls review the PR.

Copy link
Contributor

@tonilastre tonilastre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really cool feature, but there are some issues (e.g. top, or combining left vs right). Please check the comments.

The enum `NodeLabelAligment` which is used for the node `label alignment` property is defined as:

```typescript
export enum NodeLabelAligment {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A typo, missing n -> Alig + n + ment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check other usage of NodeLabelAlignment because the typo is there too.

@@ -32,6 +32,13 @@ export enum NodeShapeType {
HEXAGON = 'hexagon',
}

export enum NodeLabelAligment {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When thinking about this, I feel like this should be called NodeLabelPosition. At first (before running it), I thought this aligns the label, and the text within the label, e.g.

LEFT ALIGNMENT 
This is
multiline
text
RIGHT ALIGNMENT 
  This is
multiline
     text

So I would propose renaming this to NodeLabelPosition because it is actually a label position, rather than alignment.

Comment on lines +118 to +144
switch (node.getLabelAlignment()) {
case NodeLabelAligment.BOTTOM:
labelY += distance;
labelTextAlign = LabelTextAlign.CENTER;
labelTextBaseline = LabelTextBaseline.TOP;
break;
case NodeLabelAligment.TOP:
labelY -= distance;
labelTextAlign = LabelTextAlign.CENTER;
labelTextBaseline = LabelTextBaseline.BOTTOM;
break;
case NodeLabelAligment.LEFT:
labelX -= distance;
labelTextAlign = LabelTextAlign.RIGHT;
labelTextBaseline = LabelTextBaseline.MIDDLE;
break;
case NodeLabelAligment.RIGHT:
labelX += distance;
labelTextAlign = LabelTextAlign.LEFT;
labelTextBaseline = LabelTextBaseline.MIDDLE;
break;
default:
labelY += distance;
labelTextAlign = LabelTextAlign.CENTER;
labelTextBaseline = LabelTextBaseline.TOP;
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this should be part of Label logic. I would propose the following:

NodeLabelPosition.{TOP, BOTTOM, LEFT, RIGHT} // default = BOTTOM
NodeLabelAlignment.{LEFT, RIGHT, CENTER} // default = CENTER

EdgeLabelPosition.{CENTER} // default = CENTER
EdgeLabelAlignment.{LEFT, RIGHT, CENTER} // default = CENTER

And then Label logic receives position + alignment and handles the rest. Please take care of multiple lines because it is not working well. Check the image below for the "top".

Screenshot 2023-08-16 at 22 22 31

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node logic should just handle the start position: x, y and give that to the Label logic. Label should take the start position + label position + label alignment + text lines to figure out where each line should be.

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

Successfully merging this pull request may close these issues.

3 participants