Skip to content

Conversation

@etimberg
Copy link
Member

Creates a new helper, renderText which should take care of drawing (including stroke), strikethrough, and underlines.

If we like where this is going, I'll add some tests.

@kurkle
Copy link
Member

kurkle commented Dec 23, 2020

I like this.

@stockiNail
Copy link
Contributor

Maybe it can be used in annotation plugin for the labels

@etimberg
Copy link
Member Author

I added in some tests. I also created a simple fiddle to ensure that my bounding box methodology is correct across a wide variety of alignments and baselines. https://jsfiddle.net/4j5dqph0/

@stockiNail
Copy link
Contributor

stockiNail commented Dec 24, 2020

@etimberg @kurkle That's just a thought about the signature of renderText method.

To draw a text, you should set the canvas context font in advance, that means this implementation is assuming that whoever is using it must set the font before the call..

Furthermore the lineHeight argument is always a reference to the font property.

Why don't to pass the font instance as argument to the method in order to:

  1. set the font to canvas context
  2. use the lineHeight of the font

Something like that:

export function renderText(
  ctx: CanvasRenderingContext2D,
  text: string | string[],
  x: number,
  y: number,
  font: FontSpec,
  opts?: RenderTextOpts
): void;

In this way the method is managing all statements to render the text.
The only doubt is that setting the font outside the method, you can invoke the render avoiding setting every time the font.
Only use case as described above is the legend, where you have more texts to apply with the same font.
All other use cases have both a single text or more texts but with possible different fonts to apply (by scriptable options, for instance ticks or pointLabels).

Maybe it could add another method with the font as argument (and leaving this one as is) which will invoke the current one.

/**
 * Render text onto the canvas with font
 */
export function renderTextWithFont(ctx, text, x, y, font, opts = {}) {
   if (font) {
      ctx.font = font.string;
   }
   renderText(ctx, text, x, y, font.lineHeight, opts);
}

I don't know exactly the community approach about the granularity of the methods. Maybe the approach is do not have methods like that and delegate the caller to do this kind of stuff.

What do you think?

@etimberg
Copy link
Member Author

I think that could work @stockiNail, the reason I didn't was mostly the performance aspect (potentially setting the font more times than needed) but as you mentioned, we already have to set the for each tick individually so having that in the API would be nicer.

I would be OK to pass the font in, but if we make that change I would consider adding the styles to the RenderTextOpts as well. These would set the fill colour, stroke colour, and stroke width. We could make them optional, but it might simplify a lot of the drawing code as well.

@stockiNail
Copy link
Contributor

I would be OK to pass the font in, but if we make that change I would consider adding the styles to the RenderTextOpts as well. These would set the fill colour, stroke colour, and stroke width. We could make them optional, but it might simplify a lot of the drawing code as well.

@etimberg I fully agree! Also for those styles we should take care about the performance, as mentioned. Maybe the colors (for instance) are not used only the text but also for shapes but as you propose they could be optional and when you are in that case, the styles won't be passed.

@etimberg
Copy link
Member Author

Ok, I made some good progress. I started by moving the styling into renderText. Once that was done, I realized that the textAlign and textBaseline could go too. After that, I added translation and rotation support since we had a common pattern everywhere. Once that was all in, I moved save()/restore() into renderText as well to ensure that the method does not mutate the canvas state.

Size Change: +541 B (0%)

Total Size: 216 kB

Filename Size Change
dist/chart.esm.js 62.8 kB -149 B (0%)
dist/chart.js 78.6 kB +206 B (0%)
dist/chart.min.js 57 kB +74 B (0%)
dist/chunks/helpers.segment.js 16.2 kB +405 B (+3%)
dist/helpers.esm.js 1 kB +5 B (+1%)

@stockiNail
Copy link
Contributor

I like it! I'm looking forward for using it in annotation plugin

@etimberg etimberg merged commit 988b3c5 into chartjs:master Dec 26, 2020
@etimberg etimberg deleted the standardized-font-rendering branch December 26, 2020 16:23
@patriciadaianaboia
Copy link

I am using renderText() but seems like the values I pass to font

{
    string: 'Arial',
    family: 'Arial',
    size: 20,
    style: 'normal',
    weight: 'bold',
    lineHeight: 1.2,
  }

are not taken into consideration. Is that a bug?

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.

4 participants