New: Add support for label styling options #63

shashankshukla96 · 2023-06-05T08:33:17Z

This PR will add support to add label background styling options.

This will add style properties -

fontBackgroundBorderWidth
fontBackgroundBorderColor
fontBackgroundBorderRadius
fontBackgroundPadding

tonilastre

There is one additional bug. When using multi-line labels, there is no background. Just change the line 36 in the example-custom-styled-graph to be:

      { id: 1, label: 'Node B - A Special Node\nNew Line' },

And you will get this:

The expected behavior is to have a background under each line.

And now comes an additional question which is how border color behaves when you have one line background touching the background from the line below (e.g. padding is larger than the default line spacing) because there can be two states: A or B. What we want, is the B.

src/renderer/canvas/label.ts

tonilastre · 2023-06-09T16:10:50Z

src/renderer/canvas/label.ts

+      context.moveTo(x + borderRadius, y);
+      context.lineTo(x + width - borderRadius, y);
+      context.quadraticCurveTo(x + width, y, x + width, y + borderRadius);
+      context.lineTo(x + width, y + height - borderRadius);
+      context.quadraticCurveTo(x + width, y + height, x + width - borderRadius, y + height);
+      context.lineTo(x + borderRadius, y + height);
+      context.quadraticCurveTo(x, y + height, x, y + height - borderRadius);
+      context.lineTo(x, y + borderRadius);
+      context.quadraticCurveTo(x, y, x + borderRadius, y);
+      context.closePath();


I think you can use drawRoundRect from shapes here.

tonilastre · 2023-06-09T16:11:37Z

src/renderer/canvas/label.ts

+      context.lineTo(x, y + borderRadius);
+      context.quadraticCurveTo(x, y, x + borderRadius, y);
+      context.closePath();
+      context.fillStyle = (label.properties.fontBackgroundColor ?? DEFAULT_FONT_COLOR).toString();


Are you sure you want to use DEFAULT_FONT_COLOR here? If default font color is black, and the background is black too, then by default, the text won't be visible.

tonilastre · 2023-06-09T16:13:14Z

src/renderer/canvas/label.ts

+      context.strokeStyle = (label.properties.fontBackgroundBorderColor ?? DEFAULT_FONT_COLOR).toString();
+      context.fill();
+      context.stroke();
+    } else if (label.properties.fontBackgroundBorderWidth && !label.properties.fontBackgroundBorderRadius) {


You already have if(label.properties.fontBackgroundBorderRadius) on line 101, to the check for (!label.properties.fontBackgroundBorderRadius` is not needed here, it is irrelevant.

It is hard to figure out what will be the shape drawn from these two ifs and else so I propose splitting it so it is much easier to understand what final shape will be drawn, e.g.

// setup and preps const isBackgroundShapeDrawn = .... if (!isBackgroundShapeDrawn) { continue; } // I think this function will also handle radius = 0 to draw proper rect drawRoundRect(...); if (backgroundColor) { setFill = fill(); } If (borderWidth) { setStrokeStyle = stroke(); }

tonilastre

First of all, thanks again for this feature request. It is a really cool feature. I've added several comments because I think we can remove some parts of the code and still do the same thing - which is easier to extend later on in the future.

Let me know what do you think of the comments.

tonilastre · 2023-07-13T08:29:53Z

src/renderer/canvas/shapes.ts

+  roundedTop: Boolean = true,
+  roundedBottom: Boolean = true,


Btw when there are optional params like this, it is good to combine them into a Partial object. It is easier to extend + the code is more readable (especially when calling it), e.g.

export interface IRoundRectOptions { isTopRounded: boolean; isBottomRounded: boolean; } const DEFAULT_ROUND_RECT_OPTIONS: IRoundRectOptions = { isTopRounded: true; isBottomRounded: true; }; export const drawRoundRect = (..... r: number, options?: Partial<IRoundRectOptions>) { const config = { ...DEFAULT_ROUND_RECT_OPTIONS, ...options }; ... if (config.isTopRounded) { ... } }

And then when using this, you have the context of the object which is much easier to read:

// Before (rounded top = true) drawRoundRect(context, 100, 100, 20, false) // Before (if you just want to have rounded bottom = false, you still need to provide rounded top drawRoundRect(context, 100, 100, 20, true, false) // --- // After, when calling a function, you already know what option you are overriding drawRoundRect(context, 100, 100, 20, { isTopRounded: false }); // After, you can also just override the option you want to without noting all the previous params drawRoundRect(context, 100, 100, 20, { isBottomRounded: false }); drawRoundRect(context, 100, 100, 20, { isTopRounded: false, isBottomRounded: false });

tonilastre · 2023-07-14T13:03:28Z

src/renderer/canvas/label.ts

@@ -58,40 +72,160 @@ export class Label {
  }
 }

-export const drawLabel = (context: CanvasRenderingContext2D, label: Label) => {
+export const drawLabel = (context: CanvasRenderingContext2D, label: Label, type: 'node' | 'edge') => {


I don't really get why you need information about if it is a node or edge. Why does it matter?

Both can have multiple lines of text. As far as I can see, you probably wanted to handle the situation where padding is larger than the initial margin and in the node label case, the background color of the text would go over/under the node circle, right?

That's why we have this textBaseline property. Nodes have that set as TOP, and edges have it as MIDDLE. With that info, you have everything you need. No need to send 'node' and/or 'edge'.

tonilastre · 2023-07-14T13:03:53Z

src/renderer/canvas/edge/base.ts

      fontColor: edge.style.fontColor,
      fontFamily: edge.style.fontFamily,
      fontSize: edge.style.fontSize,
    },
  });
-  drawLabel(context, label);
+  drawLabel(context, label, 'edge');


Please check that one other comment, this 'edge' part can be removed.

tonilastre · 2023-07-14T13:04:04Z

src/renderer/canvas/node.ts

      fontColor: node.style.fontColor,
      fontFamily: node.style.fontFamily,
      fontSize: node.style.fontSize,
    },
  });
-  drawLabel(context, label);
+  drawLabel(context, label, 'node');


Please check that one other comment, this 'node' part can be removed.

tonilastre · 2023-07-14T13:04:33Z

src/renderer/canvas/label.ts

+  if (label.textLines.includes('A -> B')) {
+    console.log(borderPoints, label);
+  }


This should be removed. You probably used it for testing.

tonilastre · 2023-07-14T13:06:23Z

src/renderer/canvas/label.ts

 };

-const drawTextBackground = (context: CanvasRenderingContext2D, label: Label) => {
-  if (!label.properties.fontBackgroundColor || !label.position) {
+const drawTextBackgroundAndBorder = (


The name of this function and what it does doesn't match. It tells that it is doing the drawing, but it is actually getting the points for the background of the text.
When I get into it much deeper, those are actually bounding boxes rather than border points. But this is nitpicking.

src/renderer/canvas/label.ts

tonilastre · 2023-07-14T13:14:46Z

src/renderer/canvas/label.ts

+const calculateBorderRadius = (borderRadius: number, width: number, height: number) => {
+  // ensure that the radius isn't too large for x
+  if (width - 2 * borderRadius < 0) {
+    borderRadius = width / 2;
+  }
+
+  // ensure that the radius isn't too large for y
+  if (height - 2 * borderRadius < 0) {
+    borderRadius = height / 2;
+  }
+
+  return borderRadius;
+};


This is a cool function that could go to shapes.ts utility file, like getMaxValidBorderRadius.

tonilastre · 2023-07-14T13:27:44Z

src/renderer/canvas/label.ts

+    const borderRadius = calculateBorderRadius(fontBorderRadius, width, height);
+    drawRoundRect(context, x, y, width, height, borderRadius);
+  } else {
+    for (let i = 0; i < borderPoints.length; i++) {


I am not 100% sure why you need this logic here. I feel like all this can be done using 3 functions which are: drawRoundRect + context.fill() and context.stroke().

Here is the idea:

There are two stages: draw background and draw foreground (the actual text)

Stage 1: Background

baseline height in case if text baseline is TOP should incorporate the padding (so it doesn't overlay what is above it, e.g. node circle)

for each line of text get the bounding boxes (what you called BorderPoints) - check the context.font for context.measureText (it is in another comment)

for each line call drawRoundRect - what is cool is that it will make a path and it will not fill nor stroke it - which is great

after all lines are called with drawRoundRect we can just do 2 calls: context.stroke() (if backgroundBorderWidth > 0) and context.fill() - this fill will override the overlapping strokes.

You can test this last piece with this little sample:

<!doctype html> <html lang="en-US"> <head> <meta charset="utf-8" /> <title>Canvas</title> <style> canvas { border: 1px solid black; } </style> </head> <body> <canvas id="tutorial" width="550" height="550"></canvas> <script> function draw() { const canvas = document.getElementById("tutorial"); if (canvas.getContext) { const ctx = canvas.getContext("2d"); ctx.fillStyle = 'yellow'; ctx.lineWidth = 1; ctx.strokeStyle = 'black'; ctx.rect(100, 100, 20, 20); ctx.rect(110, 110, 20, 20); ctx.stroke(); ctx.fill(); } } draw(); </script> </body> </html>

Stage 2: Foreground

Everything should stay the same as before, the only difference is the starting y when the text baseline is TOP (in the case of the nodes) because of the padding - so the baseline height needs to incorporate the padding.

What do you think?

I think there will be one issue here if there is no backgroundColor i.e. label.properties.fontBackgroundColor is not set, in this case with the above method, we will get overlapping borders around the label. meaning if we don't apply context.fill() then it will look something like this.

So, the border has to be drawn independently of the background.

Please let me know if my understanding is not correct.

tonilastre · 2023-07-14T13:30:01Z

src/renderer/canvas/label.ts

@@ -25,6 +32,13 @@ export interface ILabelData {
  properties: Partial<ILabelProperties>;
 }

+type BorderPoint = {


Btw, we do all interfaces and types with the prefix I so we can differentiate it from the classes with the same name.

But, you already have a type you need, it is called IRectangle from common/rectangle.ts. Same fields, same types.

New: Add support for label styling options

992de9a

shashankshukla96 requested review from cizl and tonilastre as code owners June 5, 2023 08:33

tonilastre requested changes Jun 9, 2023

View reviewed changes

tonilastre added the enhancement label Jun 9, 2023

Fix: Add fix to label background features

625d34e

shashankshukla96 requested a review from tonilastre June 20, 2023 13:46

tonilastre changed the base branch from main to release/0.5.0 July 13, 2023 08:23

tonilastre requested changes Jul 14, 2023

View reviewed changes

shashankshukla96 added 2 commits July 25, 2023 18:52

Merge remote-tracking branch 'upstream/main' into label-opts

1763621

Fix: removed unnecessary variables and parameters

637dfb2

shashankshukla96 requested a review from tonilastre July 25, 2023 15:54

hal-eisen-MG added feature and removed enhancement labels Nov 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: Add support for label styling options #63

New: Add support for label styling options #63

shashankshukla96 commented Jun 5, 2023

tonilastre left a comment

tonilastre Jun 9, 2023

tonilastre Jun 9, 2023

tonilastre Jun 9, 2023

tonilastre Jun 9, 2023

tonilastre left a comment

tonilastre Jul 13, 2023

shashankshukla96 Jul 25, 2023

tonilastre Jul 14, 2023

tonilastre Jul 14, 2023

tonilastre Jul 14, 2023

tonilastre Jul 14, 2023

tonilastre Jul 14, 2023

tonilastre Jul 14, 2023

tonilastre Jul 14, 2023

shashankshukla96 Jul 25, 2023 •

edited

Loading

tonilastre Jul 14, 2023

tonilastre Jul 14, 2023

New: Add support for label styling options #63

Are you sure you want to change the base?

New: Add support for label styling options #63

Conversation

shashankshukla96 commented Jun 5, 2023

tonilastre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonilastre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashankshukla96 Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashankshukla96 Jul 25, 2023 •

edited

Loading